[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D108003#2983609 <https://reviews.llvm.org/D108003#2983609>, @xbolva00 wrote:

> I think I will start with AND only as this is more error prone pattern.

FWIW, I still see no reason //not// to warn on `|`-for-`||`. Again per https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ : if we see the programmer writing `somebool | foo`, we //know// they've messed up — they might have meant either `int(somebool) | foo` or `somebool || foo`, we're not sure //which//, but we know they've messed up //somehow//. So it makes sense to tell them about it. The codepaths for `&&/&` and `||/|` are going to be shared, right? So I see no reason to special-case-//not//-warn on `|`.
I do agree that it seems reasonable for people to screw up `&` more frequently than `|`. It's very common in C and C++ to type just a single `&` (e.g. for address-of); it's very uncommon to ever type a single `|`, so the muscle memory won't be there to typo it. Also, `|` is way off on the side of the keyboard where the typist is probably paying a little more attention; `&` is right in the hot path where our fingers are likely moving quickly. But from the compiler's POV, we might be //surprised// to see the rare `|`-for-`||` typo ("whoa! I hardly ever see one of those!") but that's no reason to keep it a secret from the user.



================
Comment at: clang/test/Sema/warn-bitwise-or-bool.c:38
+#ifdef __cplusplus
+  b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+#endif
----------------
Arguably, `foo() bitor bar()` is a sufficiently high "degree of ornamentation" to unambiguously signal the programmer's intent here. What are the chances that the programmer wrote `bitor` instead of `or` by accident?  But it's not worth adding any code just to special-case-//not//-warn here.
(The reverse — writing `or` when you meant `bitor` — strikes me as a more likely error.)
D107294 is related.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003