Removing MSVC C4800 from pragma block at the top of pybind11.h #3141
Conversation
rwgk
changed the title
| } | ||
| #endif | ||
| if (res == 0 || res == 1) { | ||
| value = (bool) res; |
There was a problem hiding this comment.
Why not just make these value = res != 0; ? The problem is that (bool) int_value is poor for performance (according to the warning), so we shouldn't be pretending this is okay to cast.
There was a problem hiding this comment.
Done. Applied globally. — I was aiming for a robust general solution before, but you're right, we only need specific solutions, and != 0 is simpler and therefore better.
rwgk
force-pushed
the
pragma_block_rm_msvc_c4800
branch
from
cb7965f to
7d7734f
Compare
rwgk
force-pushed
the
pragma_block_rm_msvc_c4800
branch
from
7d7734f to
eecd3dd
Compare
|
There is a clang-tidy check I can add to this PR which enforces this. https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html with allowing pointer to bool conversions should enforce this behavior in the future. |
|
Actually never mind, I can add that in a separate PR as that check is much more strict than this one even is and will touch a lot more code. |
Awesome, I'm glad. Combining (convoluting) things backfires too often. Best to keep things compartmentalized. |
…d#3141) * Adding PYBIND11_COMPAT_BOOL_CAST to appease MSVC 2015 warning C4800. * Replacing PYBIND11_COMPAT_BOOL_CAST with simpler != 0 * Extra parentheses (almost all compilers failed without these).
3 participants
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.