Repository navigation
gh-149044: Fix PySlot_END macro for C++ - #158866
encukou merged 4 commits into
Conversation
Sorry, something went wrong.
Initialize all fields in the macro. Otherwise, g++ -Wall -Wextra complains that some fields are not initialized [-Werror=missing-field-initializers]: error: missing initializer for member ‘PySlot::sl_flags’ error: missing initializer for member ‘PySlot::<anonymous>’ error: missing initializer for member ‘PySlot::<anonymous>’
|
Note: I found this issue while working on adding support for the limited C API to pythoncapi-compat which builds its C/C++ extension with -Wall -Wextra. See python/pythoncapi-compat#184. |
Sorry, something went wrong.
|
I created this PR to just fix PySlot_END so it can be easily backported. I also prepared draft PR #158867 which enables -Wall -Wextra in test_cext for the main branch. |
Sorry, something went wrong.
| {.sl_id=(NAME), .sl_flags=PySlot_STATIC, .sl_ptr=(VALUE)} | ||
|
|
||
| #define PySlot_END {0} | ||
| #define PySlot_END {0, 0, {0}, {0}} |
There was a problem hiding this comment.
I'm not 100% sure that this syntax works on all C/C++ compilers. In C, it's common to use {0}. A more ugly alternative is to have a separated implementation for C++:
#ifdef __cplusplus
# define PySlot_END {0, 0, {0}, {0}}
#else
# define PySlot_END {0}
#endif
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe use NULL?
| #define PySlot_END {0, 0, {0}, {0}} | |
| #define PySlot_END {0, 0, {0}, {NULL}} |
Not sure if compilers warn about this, but I heard that C++ is stricter about nullptr↔int casts.
Sorry, something went wrong.
There was a problem hiding this comment.
You might get away with
# define PySlot_END {}
for c++ (at least c++11 upwards I think). That's the idiomatic thing for "initialize everything to its default (likely 0) value". But maybe the explicit version with all members is better given it's a fairly short list of members.
Sorry, something went wrong.
|
I updated the branch to retrieve the MSan fix. |
Sorry, something went wrong.
|
@encukou @da-woods: I updated the PR to use _Py_NULL. It should C++ compiler warnings in more cases. Would you mind to review the updated PR? @da-woods: Using {} is tempting, but I also like the idea of having a single implementation to make it easier to read/maintain. Sadly, I don't think that {} is correct in C (is it?). |
Sorry, something went wrong.
|
Yeah I think a single implementation is probably cleaner. I don't believe {} would be valid C. |
Sorry, something went wrong.
encukou
left a comment
There was a problem hiding this comment.
Thank you!
({} is valid C! But not our C: it's new in C23...)
Sorry, something went wrong.
|
GH-158965 is a backport of this pull request to the 3.15 branch. |
Sorry, something went wrong.
Oh, C23 seems to be far in the future, we "just" moved to C11 for the internal C code :-) But it's great that the C standard is still evolving! Thanks for the review and the merge. Using _Py_NULL was a good idea. |
Sorry, something went wrong.
…158965) gh-149044: Avoid C++ warnings in PySlot_END macro (GH-158866) Initialize all fields in the macro. Otherwise, g++ -Wall -Wextra complains that some fields are not initialized [-Werror=missing-field-initializers]: error: missing initializer for member ‘PySlot::sl_flags’ error: missing initializer for member ‘PySlot::<anonymous>’ error: missing initializer for member ‘PySlot::<anonymous>’ (cherry picked from commit ba9d96f) Co-authored-by: Victor Stinner <vstinner@python.org>
Initialize all fields in the macro. Otherwise, g++ -Wall -Wextra complains that some fields are not initialized
[-Werror=missing-field-initializers]:
error: missing initializer for member ‘PySlot::sl_flags’
error: missing initializer for member ‘PySlot::’
error: missing initializer for member ‘PySlot::’