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

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

xbolva00 marked 2 inline comments as done and an inline comment as not done.
xbolva00 added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5609
+def warn_mul_in_bool_context : Warning<
+  "'*' in bool context, maybe you mean '&&'?">,
+  InGroup<IntInBoolContext>;
----------------
jfb wrote:
> aaron.ballman wrote:
> > xbolva00 wrote:
> > > aaron.ballman wrote:
> > > > I would appreciate seeing some motivating examples for this case, because it seems like the `&&` suggestion is misleading more often than it's helpful in the current test cases.
> > > > 
> > > > Also, what is a "bool context"?
> > > Bool context - places where we expect boolean expressions.
> > > 
> > > If you have idea for better terminology, I am happy to apply it.
> > > 
> > > Yeah, I will remove “&&” suggestion.. 
> > I don't have a good recommendation for a replacement for "boolean context", so I suppose we can leave it.
> IMO it's clearer to say "assigning the result of a multiplication to a boolean variable always yields {true|false}"
But it is very specific.. cannot be used for example here:

bool t() { return 0 * x; } 

In the place where the check is performed we know nothing whether expr is assigned/returned/etc.

I don’t believe we have to produce message with such details.


================
Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:96
+
+  if (f == apple || orange) // expected-warning {{enum constant in boolean context}}
+    return a;
----------------
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.


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

https://reviews.llvm.org/D63082