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

[JF Bastien via Phabricator via cfe-commits]

jfb 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>;
----------------
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}"


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5612
+def warn_left_shift_in_bool_context : Warning<
+  "'<<' in boolean context, maybe you mean '<'?">,
+  InGroup<IntInBoolContext>;
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > I will this line.
> * Ehh.. I will fix this line to use "did you mean". 
Same here, "assigning the result of a left shift to a boolean variable always yields {true|false}"

Same for all the other ones...


================
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 '<'?}}
+
----------------
I'm not sure the "did you mean" part is helpful. Do we have data showing that this is what people actually mean?


================
Comment at: test/Sema/warn-int-in-bool-context.c:32
+  r = a ? -2 : 0;
+  r = a ? 3 : -2;
+  r = a ? 0 : TWO; // expected-warning {{'?:' with integer constants in boolean context}}
----------------
Why wouldn't think one warn?


================
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'}}
----------------
Why does this one warn? It doesn't always yield the same result.


================
Comment at: test/Sema/warn-unreachable.c:147
+  // expected-warning at +1 {{'*' in boolean context, the expression will always evaluate to 'false'}}
   if (0 * x) calledFun(); // expected-warning {{will never be executed}}
 }
----------------
It seems like here the "will never be executed" warning is more useful. Do we want to emit both?


================
Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:99
+
+  if (ans == yes || no) // expected-warning {{enum constant in boolean context}}
+    return a;
----------------
aaron.ballman wrote:
> xbolva00 wrote:
> > aaron.ballman wrote:
> > > Why do we want to diagnose this case in C++ but not in C?
> > I think we can and should but for unknown reason (for me) this is GCC’s behaviour.
> > 
> > Maybe it is too noisy for C codebases? 
> > 
> > Maybe introduce -Wenum-in-bool-context as subgroup of -Wint-in-bool-context? And enable it for C and C++?
> I think it's just a bug in GCC (they use separate frontends for C and C++, so these sort of differences crop up sometimes). I think this should be safe to diagnose the same in C and C++.
Agreed.


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

https://reviews.llvm.org/D63082