Fix warnings for Python/ast_unparse.c

[matrixise]

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]

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

[tiran]

I'm getting slightly different warnings with GCC 7.2.1:

Python/ast_unparse.c: In function ‘append_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 isn’t a prototype [-Wstrict-prototypes]
 maybe_init_static_strings()
 ^~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c: In function ‘append_ast_expr’:
Python/ast_unparse.c:23:16: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c:119:17: note: ‘op’ was declared here
     const char *op;
                 ^~
Python/ast_unparse.c:23:16: warning: ‘op’ may be used uninitialized in this function [-Wmaybe-uninitialized]
         return _PyUnicodeWriter_WriteASCIIString(writer, charp, -1);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Python/ast_unparse.c:79:17: note: ‘op’ was declared here
     const char *op;

[matrixise]

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]

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

[tiran]

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

default: PyExc_SetString...; return -1;

[matrixise]

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]

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]

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]

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]

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

[matrixise]

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

Thanks

[tiran]

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]

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]

Thanks!