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

[Hans Wennborg via Phabricator via cfe-commits]

hans 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>;
----------------
Quuxplusone wrote:
> xbolva00 wrote:
> > Quuxplusone wrote:
> > > 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 '&'`.
> > I see the point but then I will not be able to provide -Wbool-operation-and flag to enable/disable this warning.
> > 
> > For example I know that Google prefers a new flag for every new warning so they dont have to disable eg. -Wbool-operation, but just this new warning while there are working on fixes for new warnings.
> > 
> > @hans what do you think?
> > 
> > 
> I don't understand your comment. My guess is that you didn't notice that `-Wbitwise-instead-of-logical` would still be a different flag from `-Wlogical-instead-of-bitwise`. I'm not suggesting putting them under the //same// flag; just making the newly added flag a little more consistent and (heh) logical, based on what's already there.
> @hans what do you think?

I think we're all in agreement here. Putting this behind a separate -Wbitwise-instead-of-logical flag sounds good to me.


================
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}:"&&"
----------------
xbolva00 wrote:
> Quuxplusone wrote:
> > 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
> > ```
> Yes, but oh.
> 
> This is weird, HasSideEffects is false for this case. Works fine for volatile int.
> 
> @rsmith?
I don't think a non-volatile pointer deref actually has any side effect. I think what we'd want to check here is if the expression could be reordered with the LHS. But I don't know how we'd check that.


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

https://reviews.llvm.org/D108003