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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
