aaron.ballman added inline comments.
================
Comment at: test/Sema/warn-int-in-bool-context.c:26
+ r = a << 7; // expected-warning {{'<<' in boolean context; did you mean
'<'?}}
+ r = ONE << b; // expected-warning {{'<<' in boolean context; did you mean
'<'?}}
+
----------------
jfb wrote:
> xbolva00 wrote:
> > jfb wrote:
> > > xbolva00 wrote:
> > > > jfb wrote:
> > > > > I'm not sure the "did you mean" part is helpful. Do we have data
> > > > > showing that this is what people actually mean?
> > > > No, no data, just what GCC suggests.
> > > I would drop it without knowing that it's actually what people usually
> > > mean.
> > Leave just “<<' in boolean context“ seems not ideal for me as a diag
> > message.
> >
> > Or is it ok for you?
> As [I said above](https://reviews.llvm.org/D63082#inline-585063), I don't
> think you should say "boolean context". It's not something people will
> understand. Here we're assigning to a boolean, say that.
> As I said above, I don't think you should say "boolean context". It's not
> something people will understand. Here we're assigning to a boolean, say that.
Strong +1.
As for the "did you mean", I actually see that as somewhat reasonable text.
`<<` is a reasonable typo for `<` (which yields a bool), similar for `>>` and
`>`. However, if we had data to back up that presumption, it would be better.
================
Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:96
+
+ if (f == apple || orange) // expected-warning {{enum constant in boolean
context}}
+ return a;
----------------
xbolva00 wrote:
> jfb wrote:
> > xbolva00 wrote:
> > > jfb wrote:
> > > > xbolva00 wrote:
> > > > > xbolva00 wrote:
> > > > > > @aaron.ballman : In C (but not in C++ ugh?) mode we have: 'use of
> > > > > > logical '||' with constant operand' here..
> > > > > >
> > > > > > /home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18:
> > > > > > warning: use of logical '||' with constant operand
> > > > > > if (f == apple || orange) // expected-warning {{enum constant in
> > > > > > boolean context}}
> > > > > > ^ ~~~~~~
> > > > > > /home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18:
> > > > > > note: use '|' for a bitwise operation
> > > > > > if (f == apple || orange) // expected-warning {{enum constant in
> > > > > > boolean context}}
> > > > > > ^~
> > > > > > |
> > > > > > /home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:21:
> > > > > > warning: enum constant in boolean context
> > > > > > if (f == apple || orange) // expected-warning {{enum constant in
> > > > > > boolean context}}
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > What is your opinion here (^^) , @jfb ?
> > > > >
> > > > > Thanks.
> > > > C, C++, ObjC, and ObjC++ should emit different diagnostics here. That
> > > > being said, none of these diagnostics are particular easy to
> > > > understand. I don't think we should ever say "boolean context" anywhere.
> > > So I will make this as it was - C++ only.
> > It'll work in the other languages though, right?
> Sight, making it C++ only leaves C with not diagnosed case:
>
> _Bool f() { return orange; }
>
> And I don’t know why ‘use of logical || with constant operand’ is C only :/..
> And I don’t know why ‘use of logical || with constant operand’ is C only :/..
It may be worth doing the svn blame dance to see if it's discussed as part of
the original review. One possibility is that `||` can be overloaded in C++ and
so perhaps || with a constant operand is not a bug. But that seems like pretty
weak rationale (we can always see if there's an overloaded || and not issue the
diagnostic in that circumstance).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63082/new/
https://reviews.llvm.org/D63082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
