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: allow -Wpedantic in C++20 mode #5322

Merged
merged 5 commits into from
Aug 22, 2024

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Aug 20, 2024

Description

I think we can support -Wpedandic in C++20 mode. Before that the variadic macros cause warnings that are pretty much impossible to fix from a macro AFAICT. Fix #5310.

Suggested changelog entry:

* Support ``-Wpedantic`` in C++20 mode

@henryiii henryiii force-pushed the henryiii/fix/pedantic branch 3 times, most recently from 37cd500 to 10c5398 Compare August 20, 2024 22:08
@henryiii henryiii marked this pull request as draft August 21, 2024 15:33
@henryiii henryiii force-pushed the henryiii/fix/pedantic branch 2 times, most recently from 6605ad4 to bdf929c Compare August 21, 2024 16:42
Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii changed the title fix: allow -Wpedantic again fix: allow -Wpedantic in C++20 mode Aug 21, 2024
tests/local_bindings.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!
I think it's a great compromise, to turn on the option only for C++20. That'll will catch pretty much everything already.

@@ -479,6 +479,8 @@ PYBIND11_WARNING_POP
}

\endrst */
PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_CLANG("-Wgnu-zero-variadic-macro-arguments")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just double-checking: is this needed, even with -std=c++20?

Copy link
Collaborator Author

@henryiii henryiii Aug 22, 2024

Choose a reason for hiding this comment

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

Yeah, for some reason Clang produces a warning here. GCC is fine, but Clang complains. It produces a warning saying this is a C++20 feature on newer compilers if set less than C++20, so no idea why the old warning triggers in C++20 mode. I think it's a bug.

@henryiii henryiii merged commit 9966ad4 into pybind:master Aug 22, 2024
78 checks passed
@henryiii henryiii deleted the henryiii/fix/pedantic branch August 22, 2024 04:27
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Aug 22, 2024
henryiii added a commit that referenced this pull request Aug 22, 2024
* fix: allow -Wpedantic again

Signed-off-by: Henry Schreiner <[email protected]>

* tests: ignore pedantic warning for PYBIND11_DECLARE_HOLDER_TYPE

Signed-off-by: Henry Schreiner <[email protected]>

* tests: try just turning off pedantic for one file

Signed-off-by: Henry Schreiner <[email protected]>

* tests: only run pedantic in C++20 mode

Signed-off-by: Henry Schreiner <[email protected]>

* Update tests/local_bindings.h

---------

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: token pasting of ',' and __VA_ARGS__ is a GNU extension / pedantic warnings
2 participants