-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: define PYBIND11_CPP14 for recent intel compilers #2679
fix: define PYBIND11_CPP14 for recent intel compilers #2679
Conversation
930e2d9
to
ca5494f
Compare
include/pybind11/detail/common.h
Outdated
@@ -27,7 +27,11 @@ | |||
# endif | |||
#endif | |||
|
|||
#if !(defined(_MSC_VER) && __cplusplus == 199711L) && !defined(__INTEL_COMPILER) | |||
#if defined(__INTEL_COMPILER) && __INTEL_COMPILER >= 1900 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really only added on 19? We only support 18+, so it's much simpler if it was added in 18. :'( I don't see anything specific in the release notes for 19 that indicate that they just started defining __cplusplus
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess __cplusplus
is already defined for 18, but the first comment in #1649 mentions that the C++14 support is not sufficient in that version. I will check the value of __cplusplus
in version 18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, __cplusplus
is already defined to 201402L
in version 18. So it should work in theory, but apparently it does not work in practice, see #1649. To support version 18, we thus would have to add specializations to work around the intel compiler bugs, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better than forcing C++ mode to 11 if 14 is requested. It would even be better, IMO, to throw an error if 14 is requested instead of using 11 when 14 is set, with a statement like "only C++11 is supported with Intel 18". I could make a quick attempt to get this working on Intel 18 before doing something like that.
It would be so much nicer if we could test this in CI, but only the most recent version is available and it doesn't work (#2573 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of adding workarounds to support a compiler lying about supporting C++14, though :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the minimum, let's kill the Intel specific limiting here. Since we already have a list of #error
printouts for minimum supported compilers, we could add on that forces Intel 18 to C++11 mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the first check and added an error message.
ca5494f
to
cd67921
Compare
I just encountered #1649 with the Intel compiler 2021 (beta). Recent Intel compilers seem to support C++14 just fine, so just defining
PYBIND11_CPP14
for recent Intel compilers should fix the problem.EDIT (@YannickJadoul): Closes #1649
Suggested changelog entry: