jfb 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
'<'?}}
+
----------------
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.
================
Comment at: test/Sema/warn-unreachable.c:147
+ // expected-warning@+1 {{'*' in boolean context, the expression will always
evaluate to 'false'}}
if (0 * x) calledFun(); // expected-warning {{will never be executed}}
}
----------------
xbolva00 wrote:
> jfb wrote:
> > xbolva00 wrote:
> > > jfb wrote:
> > > > It seems like here the "will never be executed" warning is more useful.
> > > > Do we want to emit both?
> > > Useful but -Wunreachable-code is disabled by default (not part of -Wall).
> > I don't think we want two diagnostics when one will do.
> I will drop mul handling then..
No, my feedback isn't about multiplication in particular. My feedback is that
it's not useful to have both warnings. We should just emit a single warning,
and it should be maximally helpful. Here it's useful to say that `calledFunc()
will never be executed because 0*x is always false`.
================
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:
> 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.
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
