[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 subscribers: mclow.lists, Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7428-7430
+def warn_bitwise_and_bool : Warning<
+  "bitwise and of boolean expressions; did you mean logical and?">,
+  InGroup<BoolOperationAnd>;
----------------
I suggest that the name and wording of this diagnostic should match `warn_logical_instead_of_bitwise`, currently `"use of logical '%0' with constant operand"`. So:
```
def warn_bitwise_instead_of_logical : Warning<
  "use of bitwise '%0' with boolean operand">,
```
This neatly sidesteps the problem of what to call the `&` operator: I was not thrilled with the phrase `bitwise and of`, but have no problem with `use of bitwise '&'`.


================
Comment at: clang/test/Sema/warn-bitwise-and-bool.c:21-22
+void test(boolean a, boolean b, int i) {
+  b = a & b;
+  b = a & foo();      // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+                      // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
----------------
So the warning triggers only when the RHS has side-effects? I'd like tests for
```
foo() & a;  // should not trigger, by that logic, because && wouldn't short-circuit anything?
(p != nullptr) & (*p == 42);  // should certainly trigger: deref is a side effect, and of course obviously this is a bug
```


================
Comment at: clang/test/Sema/warn-bitwise-and-bool.c:31
+                  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  i = !b & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+                  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"
----------------
Why is this assigning to `i` instead of `b`?
Actually, the assignment shouldn't matter to the diagnostic at all, right? Perhaps each test case should be written as, like,
```
void sink(bool);

   sink(a & b);
   sink(a & foo());
   sink(foo() & bar());
```
etc.


================
Comment at: clang/test/Sema/warn-bitwise-and-bool.c:34
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+#ifdef __cplusplus
----------------
I'd also like a negative test for @mclow.lists' example of `int n = ...; b = (n & (n-1));` (where the //result// is being used as a boolean, but the bitwise op is correct and the logical op would be 100% wrong).


================
Comment at: clang/test/Sema/warn-bitwise-and-bool.c:36
+#ifdef __cplusplus
+  b = a and foo();
+#endif
----------------
I assume you meant `bitand` and expect a warning?


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

https://reviews.llvm.org/D108003