xbolva00 marked 4 inline comments as done and an inline comment as not done.
xbolva00 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:
> > > 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?
================
Comment at: test/Sema/warn-int-in-bool-context.c:33
+ r = a ? 3 : -2;
+ r = a ? 0 : TWO; // expected-warning {{'?:' with integer constants in
boolean context}}
+ r = a ? 3 : ONE; // expected-warning {{'?:' with integer constants in
boolean context, the expression will always evaluate to 'true'}}
----------------
jfb wrote:
> xbolva00 wrote:
> > jfb wrote:
> > > Why does this one warn? It doesn't always yield the same result.
> > GCC warns here..
> Our goal isn't to achieve GCC warning parity. I like warnings that fire when
> it's extremely likely a bug. Here the diagnostic seems to be inconsistent
> with other instances of this diagnostic. Maybe this should warn, but then
> other cases should as well (such as `a?3:-2` above). Or maybe it shouldn't
> warn at all.
I will drop it and leave only
“warn_integer_constants_in_conditional_always_true” diag.
================
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}}
}
----------------
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..
================
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:
> @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.
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
