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