Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warnings for Python/ast_unparse.c #76892

Closed
matrixise opened this issue Jan 29, 2018 · 14 comments
Closed

Fix warnings for Python/ast_unparse.c #76892

matrixise opened this issue Jan 29, 2018 · 14 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@matrixise
Copy link
Member

BPO 32711
Nosy @tiran, @ambv, @serhiy-storchaka, @matrixise
PRs
  • bpo-32711: Fix warnings for Python/ast_unparse.c #5426
  • [3.7] bpo-32711: Fix warnings for Python/ast_unparse.c (GH-5426) #5475
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-02-01.17:41:54.453>
    created_at = <Date 2018-01-29.13:35:09.028>
    labels = ['interpreter-core', '3.8', 'build', '3.7']
    title = 'Fix warnings for Python/ast_unparse.c'
    updated_at = <Date 2018-02-01.17:41:54.451>
    user = 'https://github.com/matrixise'

    bugs.python.org fields:

    activity = <Date 2018-02-01.17:41:54.451>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-02-01.17:41:54.453>
    closer = 'christian.heimes'
    components = ['Interpreter Core']
    creation = <Date 2018-01-29.13:35:09.028>
    creator = 'matrixise'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32711
    keywords = ['patch']
    message_count = 14.0
    messages = ['311131', '311132', '311133', '311136', '311137', '311410', '311418', '311424', '311425', '311427', '311428', '311451', '311456', '311457']
    nosy_count = 4.0
    nosy_names = ['christian.heimes', 'lukasz.langa', 'serhiy.storchaka', 'matrixise']
    pr_nums = ['5426', '5475']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'compile error'
    url = 'https://bugs.python.org/issue32711'
    versions = ['Python 3.7', 'Python 3.8']

    @matrixise
    Copy link
    Member Author

    Python/ast_unparse.c: At top level:
    Python/ast_unparse.c:1122:1: warning: function declaration isnt a prototype [-Wstrict-prototypes]
     maybe_init_static_strings()
     ^~~~~~~~~~~~~~~~~~~~~~~~~
    Python/ast_unparse.c: In functionappend_ast_binop’:
    Python/ast_unparse.c:105:15: warning: ‘opmay be used uninitialized in this function [-Wmaybe-uninitialized]
         if (-1 == append_charp(writer, op)) {
                   ^~~~~~~~~~~~~~~~~~~~~~~~
    Python/ast_unparse.c: In functionappend_ast_unaryop’:
    Python/ast_unparse.c:132:15: warning: ‘opmay be used uninitialized in this function [-Wmaybe-uninitialized]
         if (-1 == append_charp(writer, op)) {
                   ^~~~~~~~~~~~~~~~~~~~~~~~

    @matrixise matrixise added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 29, 2018
    @tiran
    Copy link
    Member

    tiran commented Jan 29, 2018

    I'm getting slightly different warnings with GCC 7.2.1:

    Python/ast_unparse.c: In functionappend_formattedvalue’:
    Python/ast_unparse.c:859:41: warning: ordered comparison of pointer with integer zero [-Wextra]
         if (e->v.FormattedValue.format_spec > 0) {
                                             ^
    Python/ast_unparse.c: At top level:
    Python/ast_unparse.c:1122:1: warning: function declaration isnt a prototype [-Wstrict-prototypes]
     maybe_init_static_strings()
     ^~~~~~~~~~~~~~~~~~~~~~~~~
    Python/ast_unparse.c: In functionappend_ast_expr’:
    Python/ast_unparse.c:23:16: warning: ‘opmay be used uninitialized in this function [-Wmaybe-uninitialized]
             return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Python/ast_unparse.c:119:17: note: ‘opwas declared here
         const char *op;
                     ^~
    Python/ast_unparse.c:23:16: warning: ‘opmay be used uninitialized in this function [-Wmaybe-uninitialized]
             return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    Python/ast_unparse.c:79:17: note: ‘opwas declared here
         const char *op;

    @matrixise
    Copy link
    Member Author

    Hi Christian, I proposed a patch ofr the maybe_init_static_strings and for the const char *op, but I don't know how to fix for the e->v.FormattedValue.format_spec > 0, I don't know how to interpret it :/

    But for _PyUnicodeWriter_WriteASCIIString, I don't get that warning.

    @matrixise
    Copy link
    Member Author

    Hi @chris, with your help I just fixed the last warning, but I don't have the warning with _PyUnicodeWriter_WriteASCIIString :/

    @tiran
    Copy link
    Member

    tiran commented Jan 29, 2018

    the switch blocks in both functions need to handle invalid input:

    default: PyExc_SetString...; return -1;

    @matrixise
    Copy link
    Member Author

    Hi Christian,

    I add Lukasz in the loop because Victor asked to him a small review because he is not sure about the right way, PyErr_XXX or Py_UNREACHABLE.

    Thank you for your review.

    @serhiy-storchaka
    Copy link
    Member

    I think it is better to not add the default case in switch statements. If once we will add a new binary or unary operator, but forgot to update ast_unparse.c the compiler will immediately warn about this if there are no default cases. But if there is a default case the compiler will be silent.

    @matrixise
    Copy link
    Member Author

    Hi Serhiy,

    but currently, there is some warnings because op has no value and can not be assigned to NULL because _PyUnicodeWriter_WriteASCIIString can't accept a NULL value.

    What do you propose? Warnings at the compilation step and an eventual crash if we don't handle this case because there is no default?

    @tiran
    Copy link
    Member

    tiran commented Feb 1, 2018

    In my experience, Serhiy's example won't work. C doesn't guarantee that the functions will not be called with an unsupported op. Either my proposal or Victor's proposal are the correct way to solve the warning. Victor's proposal is even better because it makes the interpreter fail when an invalid operator is used.

    @matrixise
    Copy link
    Member Author

    then I will use the approach of Victor with the Py_UNREACHABLE macro and will push asap.

    @matrixise
    Copy link
    Member Author

    Christian, I just pushed my code, wait for the feedback from Travis.

    Thanks

    @tiran
    Copy link
    Member

    tiran commented Feb 1, 2018

    New changeset 83ab995 by Christian Heimes (Stéphane Wirtel) in branch 'master':
    bpo-32711: Fix warnings for Python/ast_unparse.c (bpo-5426)
    83ab995

    @tiran
    Copy link
    Member

    tiran commented Feb 1, 2018

    New changeset 78758f2 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
    [3.7] bpo-32711: Fix warnings for Python/ast_unparse.c (GH-5426) (bpo-5475)
    78758f2

    @tiran
    Copy link
    Member

    tiran commented Feb 1, 2018

    Thanks!

    @tiran tiran added the 3.8 only security fixes label Feb 1, 2018
    @tiran tiran closed this as completed Feb 1, 2018
    @tiran tiran added the build The build process and cross-build label Feb 1, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants