Removing __INTEL_COMPILER section from pragma block at the top of pybind11.h #3135
Conversation
rwgk
force-pushed
the
intel_pragma_pop
branch
2 times, most recently
from
6bfa0f1 to
82aceb7
Compare
tests/CMakeLists.txt
Outdated
| -diag-disable | ||
| 2196,11074,11076) |
There was a problem hiding this comment.
This should be added by the main CMakeLists to the correct target, not in tests. Someone using CMake (at least) should get the warnings added. ("This" being the inline/noinline one, not the max-size, as those can be handled by users)
There was a problem hiding this comment.
Could you please guide me how?
- "the main CMakeLists" — is that https://github.com/pybind/pybind11/blob/master/CMakeLists.txt?
- "correct target" — I looked through top to bottom but didn't see how to find the "correct target". Somewhere around line 184? (I'm completely lost TBH.)
I have this: https://gist.github.com/rwgk/9c4305ccc492ad78654f842f58f36b09
It shows exactly which header files trigger #2196. Is that relevant here?
There was a problem hiding this comment.
Here, actually: https://github.com/pybind/pybind11/blob/master/tools/pybind11Common.cmake
I wish the names of the targets were a bit more general - windows helper, etc. are too specific to add this too. Maybe we need an intel_helper?
There was a problem hiding this comment.
Here, actually: https://github.com/pybind/pybind11/blob/master/tools/pybind11Common.cmake
I wish the names of the targets were a bit more general - windows helper, etc. are too specific to add this too. Maybe we need an intel_helper?
Gulp ... will it be simpler like e.g.
set(clang_4plus
"$<AND:$<CXX_COMPILER_ID:Clang>,$<NOT:$<VERSION_LESS:$<CXX_COMPILER_VERSION>,3.9>>>")
set(no_register "$<OR:${clang_4plus},$<CXX_COMPILER_ID:AppleClang>>")
?
If you could help out with this part that would be really awesome.
There was a problem hiding this comment.
Wait ... thinking about it ...
This should be added by the main CMakeLists to the correct target, not in tests.
If that's what we want, we could just add this in common.h and be done with it:
#if defined(__INTEL_COMPILER)
// This pragma cannot be pop'ed:
// https://community.intel.com/t5/Intel-C-Compiler/Inline-and-no-inline-warning/td-p/1216764
# pragma warning disable 2196 // warning #2196: routine is both "inline" and "noinline"
#endif
I'll make that revision asap.
rwgk
force-pushed
the
intel_pragma_pop
branch
2 times, most recently
from
7f379b1 to
f022a3c
Compare
rwgk
changed the title
pragma warning pop for __INTEL_COMPILER.
|
Deflaking (test_iostream.py). |
|
Hi @henryiii and @Skylion007, please take another look. It's really straightforward now. |
include/pybind11/pybind11.h
Outdated
| @@ -2376,6 +2375,9 @@ inline function get_overload(const T *this_ptr, const char *name) { | |||
|
|
|||
| PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE) | |||
|
|
|||
| // | |||
| // THE `pop` HERE NEED TO BE KEPT IN SYNC WITH THE CORRESPONDING `push` AT THE TOP OF THIS FILE. | |||
There was a problem hiding this comment.
| // THE `pop` HERE NEED TO BE KEPT IN SYNC WITH THE CORRESPONDING `push` AT THE TOP OF THIS FILE. | |
| // THE `pop` HERE NEEDS TO BE KEPT IN SYNC WITH THE CORRESPONDING `push` AT THE TOP OF THIS FILE. |
IMO "push" implies "pop", otherwise you wouldn't bother to "push" in the first place, but not horribly against this note if you want it.
There was a problem hiding this comment.
I removed the notes completely. Hopefully a few PRs down the road this block will be gone entirely!
Note that I started out wanting to fix the missing pop for Intel, but in the meantime the direction of this PR changed a lot and the notes don't make that much sense anymore.
…h (it was/is only needed for 1 test).
…nstructor_stats.h.
|
Thanks for the reviews :-) |
…ind11.h (pybind#3135) * Fixing `pragma warning pop` for `__INTEL_COMPILER`. * Adding push/pop to 3 tests. Removing pybind#878 from top of pybind11.h (it was/is only needed for 1 test). * Trying again after CI failure, moving the push to the top of 2 tests. * Trying more after CI failure, adding push/pop to pybind11_tests.h, constructor_stats.h. * Moving ICC pybind#2196 suppression to CMakeLists.txt * Fixing condition for `pragma GCC diagnostic push` in pybind11.h * Moving `pragma warning disable 2196` to common.h * Revising #ifdef to be more conservative. * Undoing insertion of notes that will hopefully soon be completely obsolete anyway.
Follow-on to PR #3127, based on results obtained under PR #3125.
The suggested changelog entry will be in the final PR of this cleanup series.