-
Notifications
You must be signed in to change notification settings - Fork 2.2k
More systematic gcc & clang coverage #4083
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
Conversation
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 looks good to me to me on the CI front, but I was confused why the pragrams are still in the include if the std::move is removed?
|
||
if (size == 0) { | ||
return std::move(result); | ||
return result; | ||
} |
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.
If you remove the std::move are the pragmas necessary here, anymore?
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.
Here a GCC 11 & 12 warning is at conflict with a clang 7 through 12 warning.
When GCC 11 & 12 complained, I simply removed the std::move
. (Thinking: "Let's see what happens.")
Next CI: many clang failures.
Next CI: complete the clang coverage to better understand what is going on.
Looking at the results I concluded (guessed TBH): clang was wrong for a long time, but that was fixed in clang 13. The fix appears to stick: clang 14 & 15 agree.
Next CI: with pragmas for clang added.
I considered introducing something like
PYBIND11_SUPPRESS_MISLEADING_CLANG_WARNING_CALL_STD_MOVE_EXPLICITLY_BEGIN
...
PYBIND11_SUPPRESS_MISLEADING_CLANG_WARNING_CALL_STD_MOVE_EXPLICITLY_END
or
return PYBIND11_KNOWN_REDUNDANT_STD_MOVE(result);
but decided against it because there are only two affected locations, and the implementation you see now is easier to understand just looking at the local code, without having to look elsewhere.
@henryiii I'll merge this now, to have this included in the next smart_holder update. Please let me know any suggestions, I'll work on them in a follow-on PR. @Skylion007 thanks for reviewing! |
Description
Based on #4074 (comment)
Drops GCC 10
--std=c++2a
(because it is an odd choice), adds 6 GCC /-std
combinations not covered already but assumed to be what users would pick.Also adds clang 11, 12, 13 (not covered at all before), with
--std=c++20
.Resolves GCC 11 & 12 "redundant move in return statement" warnings.
Suppresses misleading clang warnings in the same locations.
Suggested changelog entry: