[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.


================
Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:96
+
+  if (f == apple || orange) // expected-warning {{enum constant in boolean context}}
+    return a;
----------------
aaron.ballman wrote:
> 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).
But even if we fix ‘use of logical .. with constants” for C++, we the break this patch (clang will produce two similar warnings) and if we remove it from this patch, some cases stay undiagnosed:

bool foo() return orange;

In order to support this case, we cant use this place to implement the check.

Not good..


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

https://reviews.llvm.org/D63082