Removing MSVC C4127 from pragma block at the top of pybind11.h

[rwgk]

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.

Worksheet

[henryiii]

What about something like:

// if C++17+
#if defined(PYBIND11_CPP17)
#    define PYBIND11_CONSTEXPR_IF(arg) if constexpr (arg)
#else
constexpr inline bool constexpr_bool(bool cond) { return cond; }
#    define PYBIND11_CONSTEXPR_IF(arg) pybind11::detail::constexpr_bool(arg)
#endif

This way we get a little benefit from this, where the current version is just a hack to quiet the warning.

[Skylion007]

Just copy this machinery from this experimental PR: #3113

#if defined(PYBIND11_CPP17)
#    define PYBIND11_CPP17_CONSTEXPR constexpr
#else
#    define PYBIND11_CPP17_CONSTEXPR
#endif

[henryiii]

Does this warning show up for C++ < 17? It really shouldn't, as there's not an easy "fix" there. But I expect it does?

[henryiii]

That wouldn't fix the warning if this is produced in C++11 or C++14 mode.

[henryiii]

if PYBIND11_CPP17_CONSTEXPR (constexpr_bool(...)) is not horrible, though, that is an option.

[henryiii]

if constexpr also avoids checking the condition at runtime, and ensures that a runtime "if" is not placed in the code. So if it reduces to if constexpr(True) then the else block is not included in the compilation. While if(True) still compiles the else. So you get a small benefit. We likely will not be dropping C++14 (or even 11, possibly) support for a long time, so being able to simplify this would be a nice side benefit. MSVC is correctly indicating this should be if constexpr with the warning; we just can't require it. It's actually really helpful, because it tells us exactly what if statements are constant without having to read the code.

The compiler may turn a runtime if into a compile time if if the expression is simple enough. In fact, MSVC doesn't warn if there's a simple "true" or similar in the statement. For example, this is a compile time if on all GCC optimization levels:

int check(int j) {
    if (true)
      return 2*j;
    else
      return 3*j;
}

However, this is not a compile time if on GCC if optimization is off (-O0):

constexpr bool f(bool x) {return x;}

int check(int j) {
    if (f(true))
      return 2*j;
    else
      return 3*j;
}

This is, even at -O0:

constexpr bool f(bool x) {return x;}

int check(int j) {
    if constexpr (f(true))
      return 2*j;
    else
      return 3*j;
}

See https://gcc.godbolt.org/z/WhzndzTTx

It's actually a very big deal

IIRC, it actually doesn't work in quite as many places as I would have wanted it too, but yes, it's very, very useful for this. Dramatically simplifies SFINAE (most of the time).

Given that this usually optimizes to the same thing, I'm fine with not adding the constexprs.

[henryiii]

(Though adding PYBIND11_CPP17_CONSTEXPR on all these in a followup might be nice)

[henryiii]

Note that the constexpr_bool is only for a subexpression.

You could just as easily apply it to the whole expression. The other part is constant too (it has to be to trigger the warning).

[henryiii]

No, I take that back - why would that trigger the warning then? A constant expression combined with a non-constant one triggers the warning?

[rwgk]

No, I take that back - why would that trigger the warning then? A constant expression combined with a non-constant one triggers the warning?

I think what happened: sometime in the 1990s or even 80s someone implemented C4127 for C. It made total sense: the warning would tell you that you could simply remove the (sub)expression (and maybe an entire if or else branch with it). Then templates were added to the language. But nearly a quarter century later, Microsoft still hasn't updated the diagnostic to take that into account, therefore in modern template heavy code most of these warnings are false positives, irritating noise.

I don't think it's useful to convolute constexpr modernization into dealing with the antiquated warning, beyond the coincidence-to-some-degree that a constexpr function can be used as a trick to silence the warning.

[rwgk]

I'm outside and cannot look carefully right now, but please click on the Worksheet link in the PR description for the full ground truth. From memory: every MSVC ever in existence, and in any mode we use, produces that warning.

…

On Wed, Jul 28, 2021 at 20:05 Henry Schreiner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/pybind11/detail/common.h <#3152 (comment)>: > @@ -926,5 +926,8 @@ inline constexpr void workaround_incorrect_msvc_c4100(Args &&...) {} # define PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(...) #endif +// Suppresses MSVC warning C4127: Conditional expression is constant +constexpr inline bool constexpr_bool(bool cond) { return cond; } The only goal of the constexpr_bool is to remove this warning. Most compilers don't warn here, since it's not easy to avoid an if with a static condition until C++17. That's why I'm not wildly excited about adding constexpr_bool; using a macro that can also utilize if constexpr would be an actual possible benefit. if constexpr(constexpr_bool(...)) will be constexpr, but there's no difference with if constexpr(...), just for hiding this warning when the "constexpr" is not present. That's why I'd be much more tempted if MSVC doesn't produce this warning in C++11/14 mode, but I *think* it does? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3152 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZADKWVMUA4FZAKREXCDT2DAQLANCNFSM5BFNNTTA> .

[rwgk]

Awesome, thanks Henry!
I'm just in the middle of rebasing: this PR will now remove the entire MSVC section of the pragma block!
Only two GCC warnings left to do.