Introduce a new style of warning suppression based on push/pop #4285

EthanSteinberg · 2022-10-25T14:28:01Z

Description

As I was working through #4250, I realized that pybind11's warning suppression system is a bit too fragile. We can't globally disable any of the useless warnings like C4127 and it requires repeated compiler version checks in a lot of cases scattered throughout the code.

This PR introduces 2 things:

A common PYBIND11_PRAGMA macro for allowing the generic use of _Pragma across platforms. This will allow us to write platform generic #pragma macros.
A warning push and pop at the beginning of every PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END

As pushes and pops are error-prone, the general plan should be to rely on PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END to do the required pushing and popping of warnings.

As an aside, I would also like to argue that we should globally disable C4127 (constant expression in if statement), but I think this PR is still useful even if we don't globally disable it.

Skylion007 · 2022-10-25T14:55:09Z

@rwgk Went through a lot of trouble in a previous PR to remove all these warning pop push pragma warnings as they were bugprone for accidentally silencing warnings in downstream projects. So we try to avoid pushing / pop warning pragmas unless we don't have a choice.

EthanSteinberg · 2022-10-25T15:03:04Z

@rwgk Went through a lot of trouble in a previous PR to remove all these warning pop push pragma warnings as they were bugprone for accidentally silencing warnings in downstream projects. So we try to avoid pushing / pop warning pragmas unless we don't have a choice.

I changed it so that the push and pop are done at PYBIND11_NAMESPACE_BEGIN and PYBIND11_NAMESPACE_END

We will never need to push and pop manually and so can't make mistakes.

Skylion007 · 2022-10-25T15:18:41Z

@rwgk Thoughts on this?

rwgk

That is so cool! I didn't have the idea to tag the _Pragma onto PYBIND11_NAMESPACE when I was going through my round of cleanup.

I'm 100% for globally disabling the C4127 warning with your trick. It's just useless, almost harmful because of all the clutter it creates suppressing it locally. We already had discussions about that in the past.

I'll look more later. Today I'm very busy.

rwgk · 2022-10-25T15:47:50Z

JIC you're interested in historical background, how we got to what you see:

https://docs.google.com/spreadsheets/d/1XDKkq1w7d9f2dPcVsn-vmZHleMWQkVCl7xb4cilpGiI/edit#gid=1527078177

Column R in that sheet has a list of PRs.

EthanSteinberg · 2022-10-25T16:27:17Z

JIC you're interested in historical background, how we got to what you see:

https://docs.google.com/spreadsheets/d/1XDKkq1w7d9f2dPcVsn-vmZHleMWQkVCl7xb4cilpGiI/edit#gid=1527078177

Column R in that sheet has a list of PRs.

Yep, if this PR gets merged I would want to do a general cleanup of all warnings under this new system in a new PR.

We would globally disable some, and swap others to other APIs as needed.

include/pybind11/cast.h

rwgk · 2022-10-25T17:09:42Z

We would globally disable some, and swap others to other APIs as needed.

Sounds awesome. I will look carefully here asap.

It's not easy with git: if you branch from this branch to do a little bit of the follow-on steps, that might be useful for us to see exactly where it's going.

EthanSteinberg · 2022-10-25T17:31:13Z

Doing a quick pass, it looks like we would only want to globally disable Intel 2196 and MSVC C4127.

The rest are good warnings, but should still be cleaned up quite a bit.

The following regex gives all the lines that should be cleaned up:

#\s*pragma (?!once)

rwgk · 2022-11-02T21:50:19Z

The force push was just for rebase on current master. I didn't change anything.

rwgk · 2022-11-02T22:34:32Z

I wanted to see what it's like to work with the new system, practicing with e94b70f. It's great!

Did you have someone special in mind for globally disabling that particular warning? — After my commit, there are only 6 header files and 2 test files that need PYBIND11_DISABLE_WARNING_MSVC(4127), seems fine to me.

Feel free to git revert my commit if you think it is going in the wrong direction (but please do not remove it completely, so we can keep referencing it).

rwgk · 2022-11-03T04:42:52Z

Works like a charm!

There isn't that much more work in it (below). Looks like ~20-30 minutes of leg work, then mostly waiting for the CI and maybe fixing oversights. What do you think about doing that in this PR? I'd be happy to help if you want to split the work. It almost looks like too much fun getting rid of all those noisy pragma blocks.

$ grr -P '#\s*pragma (?!once)'
include/pybind11/pytypes.h:#    pragma warning(push)
include/pybind11/pytypes.h:#    pragma warning(disable : 4522) // warning C4522: multiple assignment operators specified
include/pybind11/pytypes.h:#    pragma warning(pop)
include/pybind11/pybind11.h:#    pragma GCC diagnostic push
include/pybind11/pybind11.h:#    pragma GCC diagnostic ignored "-Wnoexcept-type"
include/pybind11/pybind11.h:#    pragma GCC diagnostic push
include/pybind11/pybind11.h:#    pragma GCC diagnostic ignored "-Wplacement-new"
include/pybind11/pybind11.h:#    pragma GCC diagnostic pop
include/pybind11/pybind11.h:#    pragma GCC diagnostic push
include/pybind11/pybind11.h:#    pragma GCC diagnostic ignored "-Wstrict-aliasing"
include/pybind11/pybind11.h:#    pragma GCC diagnostic pop
include/pybind11/pybind11.h:#    pragma GCC diagnostic pop // -Wnoexcept-type
include/pybind11/numpy.h:#    pragma clang diagnostic push
include/pybind11/numpy.h:#    pragma clang diagnostic ignored "-Wreturn-std-move"
include/pybind11/numpy.h:#    pragma clang diagnostic pop
include/pybind11/eigen/matrix.h:#    pragma warning(push)
include/pybind11/eigen/matrix.h:#    pragma warning(disable : 5054) // https://github.com/pybind/pybind11/pull/3741
include/pybind11/eigen/matrix.h:#    pragma GCC diagnostic push
include/pybind11/eigen/matrix.h:#    pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
include/pybind11/eigen/matrix.h:#    pragma warning(pop)
include/pybind11/eigen/matrix.h:#    pragma GCC diagnostic pop
include/pybind11/eigen/tensor.h:#    pragma warning(push)
include/pybind11/eigen/tensor.h:#    pragma warning(disable : 4554) // Tensor.h warning
include/pybind11/eigen/tensor.h:#    pragma warning(disable : 4127) // Tensor.h warning
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic push
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
include/pybind11/eigen/tensor.h:#    pragma warning(pop)
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic pop
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic push
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic ignored "-Wtype-limits"
include/pybind11/eigen/tensor.h:#    pragma GCC diagnostic pop
include/pybind11/detail/common.h:#    pragma warning disable 2196 // warning #2196: routine is both "inline" and "noinline"
include/pybind11/detail/common.h:#    pragma warning(push)
include/pybind11/detail/common.h:#    pragma warning(disable : 4505)
include/pybind11/detail/common.h:#    pragma warning(pop)
tests/test_constants_and_functions.cpp:#    pragma warning push
tests/test_constants_and_functions.cpp:#    pragma warning disable 878 // incompatible exception specifications
tests/test_constants_and_functions.cpp:#    pragma warning pop
tests/test_kwargs_and_defaults.cpp:#    pragma clang diagnostic push
tests/test_kwargs_and_defaults.cpp:#    pragma clang diagnostic ignored "-Wreturn-std-move"
tests/test_kwargs_and_defaults.cpp:#    pragma clang diagnostic pop
tests/test_embed/test_interpreter.cpp:#    pragma warning(disable : 4996)
tests/test_embed/catch.cpp:#    pragma warning(disable : 4996)
tests/test_eigen_matrix.cpp:#    pragma warning(disable : 4996) // C4996: std::unary_negation is deprecated
tests/test_operator_overloading.cpp:#    pragma GCC diagnostic push
tests/test_operator_overloading.cpp:// Here, we suppress the warning using `#pragma diagnostic`.
tests/test_operator_overloading.cpp:#            pragma GCC diagnostic ignored "-Wself-assign-overloaded"
tests/test_operator_overloading.cpp:#            pragma GCC diagnostic ignored "-Wself-assign-overloaded"
tests/test_operator_overloading.cpp:#    pragma GCC diagnostic pop
tests/test_class.cpp:#    pragma warning(disable : 4324)

EthanSteinberg · 2022-11-03T14:00:17Z

Thanks for taking a look and finishing the cleanup for that file!

Did you have someone special in mind for globally disabling that particular warning? — After my commit, there are only 6 header files and 2 test files that need PYBIND11_DISABLE_WARNING_MSVC(4127), seems fine to me.

We should be able to just add our global disables to PYBIND11_NAMESPACE_BEGIN

There isn't that much more work in it (below). Looks like ~20-30 minutes of leg work, then mostly waiting for the CI and maybe fixing oversights. What do you think about doing that in this PR? I'd be happy to help if you want to split the work. It almost looks like too much fun getting rid of all those noisy pragma blocks.

Yep, updating them all in this PR is probably a good idea. I'll do that this weekend.

The real question is whether to introduce PYBIND11_WARNING_BLOCK macros or to manually push/pop.

rwgk · 2022-11-03T14:29:24Z

We should be able to just add our global disables to PYBIND11_NAMESPACE_BEGIN

I have a split mind about that: what we have right now is very low noise, lets us know in which files the C4127 suppression is actually needed, and doubles as a small reminder, to be less surprised when we need to add it to some new test in the future.

Tagging it unconditionally to PYBIND11_NAMESPACE_BEGIN is only very slightly more convenient.

That convenience may (mis)lead (IMO) people to dubious PYBIND11_NAMESPACE_BEGIN/END uses.

I'm fine either way though.

The real question is whether to introduce PYBIND11_WARNING_BLOCK macros or to manually push/pop.

Trying to understand:

With this PR as is I'd use (actually, your code)

    PYBIND11_WARNING_PUSH
    PYBIND11_DISABLE_WARNING_GCC(-Wdeprecated)
    PYBIND11_DISABLE_WARNING_CLANG(-Wdeprecated)
    ... code that needs it is here ...
    PYBIND11_WARNING_POP

That's already far better than what we have right now. Is PYBIND11_WARNING_BLOCK for this situation, to make that even better? (How? it doesn't click for me TBH)

EthanSteinberg · 2022-11-03T14:35:14Z

That's already far better than what we have right now. Is PYBIND11_WARNING_BLOCK for this situation, to make that even better? (How? it doesn't click for me TBH)

PYBIND11_WARNING_BLOCK would take advantage of C++ scopes to force users to unpop as otherwise the compiler would throw an error.

The way it would be implemented is:

#define PYBIND11_START_WARNING_BLOCK PYBIND11_WARNING_PUSH {

#define PYBIND11_END_WARNING_BLOCK PYBIND11_WARNING_POP }

Example code

{ // Note that you must start each WARNING_BLOCK with a {
    START_WARNING_BLOCK
    
    PYBIND11_DISABLE_WARNING_CLANG(-Wdeprecated)
    // blah blah blah, code that triggers warning
    
    END_WARNING_BLOCK 
}

If users forget to include END_WARNING_BLOCK, the compiler will throw an error.

Compare that to our current setup, where the following will still compile

WARNING_PUSH
PYBIND11_DISABLE_WARNING_CLANG(-Wdeprecated)
// blah blah blah, code that triggers warning

// We forgot to POP, this will break our warning suppression system in a really bad way!

rwgk · 2022-11-03T14:44:22Z

PYBIND11_WARNING_BLOCK would take advantage of C++ scopes

I'm a bit worried about using scopes: that could easily lead to accidents, too, variables getting shadowed. I think clang-tidy or some compilers warn or error out, but not all. I've lost some time over such mistakes on and off.

Detecting missing pops could be an easy pre-commit hook.

We could fold in detecting PYBIND11_NAMESPACE_BEGIN/END mismatches, which I was wishing for already, but never got to it. (I think I had a PR fixing a couple such oversights, but not worth digging up that number right now.)

EthanSteinberg · 2022-11-03T14:47:29Z

Detecting missing pops could be an easy pre-commit hook.

Yeah, I think you are right. PYBIND11_WARNING_BLOCK is probably not worth it since we can replace it with a very simple pre-commit hook. We can always add it later if the manual push/pops become a problem.

I'll stick with the current design:

No special global disables at namespace_begin. We'll just disable in each file as needed.
Manual pushes / pops for special cases.

EthanSteinberg · 2022-11-21T03:35:04Z

@rwgk Sorry for the delay, but I just cleared out the remaining #pragmas and fixed all the other issues. I think this PR is good to go!

include/pybind11/detail/common.h

rwgk

Awesome! Just one naming suggestion.

(I dread the thought of resolving the merge conflicts with the smart_holder branch, but it's totally worth it!)

EthanSteinberg · 2022-11-22T02:07:15Z

I just rebased this off the current master, so it should be ready for merging!

rwgk

Wonderful!

@ax3l
@EricCousineau-TRI
@Skylion007

This PR has a lot of sprawling changes and will therefore bit-rot pretty quickly (if we merge other things).

Could you please help out getting this reviewed and merged soon? I believe this is a quick review:

The only file that needs careful review is pybind11/detail/common.h, and even that is pretty repetitive (sections for each type of compiler).
All the rest is boilerplate. It seems very unlikely that a serious issue with those changes could pass our extensive testing. I think if a couple more people spend 5 minutes each glancing through there isn't much that could still slip in accidentally.

include/pybind11/detail/common.h

EricCousineau-TRI

Looks awesome! Just a small nit-pick request about guidance for future devs.

include/pybind11/detail/common.h

rwgk · 2022-11-28T15:39:28Z

The only one CI failure is an unrelated known flake (CUSTOMBUILD : error : RPC failed; curl 92 HTTP/2 stream 0 was not closed cleanly: INTERNAL_ERROR ). Ignoring.

Thanks @lalaland for this great work, thanks @EricCousineau-TRI for the review!

…ased on push/pop)

rwgk · 2023-02-17T18:22:44Z

Hi @lalaland, coincidentally, my attention got drawn to this change:

https://github.com/pybind/pybind11/pull/4285/files#diff-2830f572a116e44a5c7730a1e6d0ed17bb547b8c6186276ba482bcb01c4c8cddL29-L32

For easy reference, that diff is:

-#elif defined(__MINGW32__)
-#    pragma GCC diagnostic push
-#    pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
-#endif
+PYBIND11_WARNING_DISABLE_GCC("-Wmaybe-uninitialized")

IIUC, before this PR that warning suppression was specific to MINGW, but now it applies to all GCCs. Is that a correct understanding? Do you have ideas for making it specific again in a nice way? I guess I could simply put back #if defined(__MINGW32__), and only if we need more of those later try to be smarter?

EthanSteinberg · 2023-02-17T18:50:29Z

IIUC, before this PR that warning suppression was specific to MINGW, but now it applies to all GCCs. Is that a correct understanding? Do you have ideas for making it specific again in a nice way? I guess I could simply put back #if defined(MINGW32), and only if we need more of those later try to be smarter?

Feel free to add the #if statement for MINGW32.

Alternatively, if we have a lot of these, might be worth considering PYBIND11_WARNING_DISABLE_MINGW32

Background: pybind#4285 (comment)

Background: #4285 (comment)

EthanSteinberg marked this pull request as draft October 25, 2022 14:34

EthanSteinberg changed the title ~~[draft] Switch to an alternative style of C4127 warning suppression~~ Introduce a new style of warning suppression based on push/pop Oct 25, 2022

EthanSteinberg marked this pull request as ready for review October 25, 2022 15:32

rwgk reviewed Oct 25, 2022

View reviewed changes

Skylion007 reviewed Oct 25, 2022

View reviewed changes

include/pybind11/cast.h Outdated Show resolved Hide resolved

rwgk force-pushed the disable_c4127 branch from 4a7342e to 56cddea Compare November 2, 2022 21:50

rwgk reviewed Nov 21, 2022

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

rwgk approved these changes Nov 21, 2022

View reviewed changes

EthanSteinberg added 2 commits November 22, 2022 01:37

Introduce a new warning suppression system

ac21ed4

Switch to better name

f64bc2c

EthanSteinberg force-pushed the disable_c4127 branch from f12191a to f64bc2c Compare November 22, 2022 01:40

rwgk approved these changes Nov 23, 2022

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

EricCousineau-TRI approved these changes Nov 28, 2022

View reviewed changes

include/pybind11/detail/common.h Outdated Show resolved Hide resolved

Nits

9ea41fd

rwgk merged commit 06003e8 into pybind:master Nov 28, 2022

github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 28, 2022

rwgk removed the needs changelog Possibly needs a changelog entry label Nov 28, 2022

rwgk mentioned this pull request Dec 1, 2022

Add support for nested exceptions in custom exceptions #4366

Open

rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 1, 2022

Changes related to PR pybind#4285 (new style of warning suppression b…

6752b6c

…ased on push/pop)

rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 1, 2022

Changes related to PR pybind#4285 (new style of warning suppression b…

401def5

…ased on push/pop)

rwgk added a commit to rwgk/pybind11 that referenced this pull request Dec 2, 2022

Changes related to PR pybind#4285 (new style of warning suppression b…

13eb5ea

…ased on push/pop)

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11clif#4285

Closed

rwgk added a commit to rwgk/pybind11 that referenced this pull request Feb 17, 2023

Make warning suppressions MINGW-specific again.

c27e691

Background: pybind#4285 (comment)

rwgk mentioned this pull request Feb 17, 2023

Make warning suppressions MINGW-specific again. #4515

Merged

rwgk added a commit that referenced this pull request Feb 17, 2023

Make warning suppressions MINGW-specific again. (#4515)

6a5e600

Background: #4285 (comment)

henryiii pushed a commit that referenced this pull request Feb 24, 2023

Make warning suppressions MINGW-specific again. (#4515)

9a1eeed

Background: #4285 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a new style of warning suppression based on push/pop #4285

Introduce a new style of warning suppression based on push/pop #4285

EthanSteinberg commented Oct 25, 2022 •

edited

Loading

Skylion007 commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022 •

edited

Loading

Skylion007 commented Oct 25, 2022

rwgk left a comment

rwgk commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022

rwgk commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022

rwgk commented Nov 2, 2022

rwgk commented Nov 2, 2022 •

edited

Loading

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022 •

edited

Loading

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022

EthanSteinberg commented Nov 21, 2022 •

edited

Loading

rwgk left a comment

EthanSteinberg commented Nov 22, 2022 •

edited

Loading

rwgk left a comment

EricCousineau-TRI left a comment

rwgk commented Nov 28, 2022

rwgk commented Feb 17, 2023

EthanSteinberg commented Feb 17, 2023

Introduce a new style of warning suppression based on push/pop #4285

Introduce a new style of warning suppression based on push/pop #4285

Conversation

EthanSteinberg commented Oct 25, 2022 • edited Loading

Description

Skylion007 commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022 • edited Loading

Skylion007 commented Oct 25, 2022

rwgk left a comment

Choose a reason for hiding this comment

rwgk commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022

rwgk commented Oct 25, 2022

EthanSteinberg commented Oct 25, 2022

rwgk commented Nov 2, 2022

rwgk commented Nov 2, 2022 • edited Loading

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022 • edited Loading

rwgk commented Nov 3, 2022

EthanSteinberg commented Nov 3, 2022

EthanSteinberg commented Nov 21, 2022 • edited Loading

rwgk left a comment

Choose a reason for hiding this comment

EthanSteinberg commented Nov 22, 2022 • edited Loading

rwgk left a comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

rwgk commented Nov 28, 2022

rwgk commented Feb 17, 2023

EthanSteinberg commented Feb 17, 2023

EthanSteinberg commented Oct 25, 2022 •

edited

Loading

EthanSteinberg commented Oct 25, 2022 •

edited

Loading

rwgk commented Nov 2, 2022 •

edited

Loading

EthanSteinberg commented Nov 3, 2022 •

edited

Loading

EthanSteinberg commented Nov 21, 2022 •

edited

Loading

EthanSteinberg commented Nov 22, 2022 •

edited

Loading