fix: make PYBIND11_WARNING_POP actually pop clang diagnostics #5448
Conversation
|
Oh wow, thanks for catching this! I'm amazed how long that oversight survived. Could you please help fixing up the C++20 failures? From a quick search, I think this should work as a fix: I think you can use |
J-M0
force-pushed
the
fix-clang-pop
branch
3 times, most recently
from
d6d7c7a
to
47b8c14
Compare
|
I'm not sure if this is the right solution, but it's the best result I've gotten so far |
|
To expand a little more, until C++20, the standard required that at least one value be given for the
|
|
Oh sorry, what I had in mind is changing the C++ code to conditionally use the C++20 feature. (No cmake changes.) I'll give it a try. |
This reverts commit d310959.
…CLARE_HOLDER_TYPE()`
… rwgk why it does not work).
This is the beginning of the error message:
```
D:\a\pybind11\pybind11\tests\test_smart_ptr.cpp(285,1): error C2143: syntax error: missing ')' before ',' [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
D:\a\pybind11\pybind11\tests\test_smart_ptr.cpp(285,1): error C2059: syntax error: ')' [D:\a\pybind11\pybind11\build\tests\pybind11_tests.vcxproj]
```
…nts")` in test_smart_ptr.cpp
This is the error message:
```
/__w/pybind11/pybind11/tests/test_smart_ptr.cpp:287:51: error: must specify at least one argument for '...' parameter of variadic macro [-Werror,-Wgnu-zero-variadic-macro-arguments]
PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>)
^
/__w/pybind11/pybind11/include/pybind11/cast.h:885:13: note: macro 'PYBIND11_DECLARE_HOLDER_TYPE' defined here
^
```
…rguments")` in test_virtual_functions.cpp
…rguments")` in test_embed/test_interpreter.cpp
…uments")` near the top of pybind11/pybind11.h; 2. Change `PYBIND11_DECLARE_HOLDER_TYPE` macro to side-step the only remaining clang warning-as-error (this is still needed even for clang 18). Alternatively the warning suppression could be moved into pybind11/cast.h, but this commit limits the warning suppression to smaller scope within include/pybind11.
|
My idea that
To get around both problems, I believe it's a useful compromise to
|
|
Yeah I ran into those same problems too, but I see I didn't articulate that well. I did try to ignore the warning globally by adding it to So how do we want to go from here? Squash this PR down to two commits? |
We always use Squash and merge. While working on a PR, I find it very useful to never force push, and to leave a comment with each commit to explain the next step. That way it's easy for others to follow the progress, and also easier to backtrack. I posted a review request in a chat room where other maintainers hang out. I'll wait a few days to see if anyone sends feedback. |
Description
PYBIND11_WARNING_POPis defined for clang asPYBIND11_PRAGMA(clang diagnostic push), meaning ignored diagnostics don't actually get reset.Suggested changelog entry: