Skip to content

gh-149044: Fix PySlot_END macro for C++ - #158866

Open
vstinner wants to merge 4 commits into
python:mainfrom
vstinner:pyslot_end
Open

vstinner wants to merge 4 commits into
python:mainfrom
vstinner:pyslot_end

Conversation

@vstinner

@vstinner vstinner commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

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::’

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>’
@vstinner vstinner added topic-C-API needs backport to 3.15 pre-release feature fixes, bugs and security fixes labels Oct 5, 2026
@bedevere-app bedevere-app Bot added the type-feature A feature request or enhancement label Oct 5, 2026
@vstinner

vstinner commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

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.

@vstinner vstinner removed the type-feature A feature request or enhancement label Oct 5, 2026
@vstinner

vstinner commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

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.

Comment thread Include/slots.h Outdated
{.sl_id=(NAME), .sl_flags=PySlot_STATIC, .sl_ptr=(VALUE)}

#define PySlot_END {0}
#define PySlot_END {0, 0, {0}, {0}}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use NULL?

Suggested change
#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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bedevere-app bedevere-app Bot added the type-feature A feature request or enhancement label Oct 5, 2026
@vstinner

vstinner commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

I updated the branch to retrieve the MSan fix.

@vstinner

vstinner commented Oct 5, 2026

Copy link
Copy Markdown
Member Author

cc @encukou

@vstinner

vstinner commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

@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?).

@da-woods

da-woods commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Yeah I think a single implementation is probably cleaner. I don't believe {} would be valid C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core review needs backport to 3.15 pre-release feature fixes, bugs and security fixes topic-C-API type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants