[PATCH] D58878: [Diagnostics] Warn for assignments in bool contexts

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6738
+  "assignment to bool">,
+  InGroup<AssigmentInBoolContext>;
 // Completely identical except off by default.
----------------
xbolva00 wrote:
> rjmccall wrote:
> > Sorry, I didn't mean to suggest that you should change the existing diagnostic.  You should add a second diagnostic in a new warning group (which should be implied by `-Wparentheses`) that you use just in this case.
> > 
> > We try to add new warnings this way — even when they're just generalizations of existing warnings — so that they can be independently disabled by e.g. the compiler teams at Apple and Google that need to periodically roll out new compilers across a large codebase.
> This is quite problematic to isolate this one since tha patch add DiagnoseAssignmentAsCondition call into the PerformImplicitConversion so I don't know how to see in PerformImplicitConversion whether we are in the condition or not. So, this new warning note wins over current one "using the result of an assignment as a condition without parentheses" always :/ 
`PerformImplicitConversion` is not being used for the expression-as-condition path.  You can tell because otherwise we'd be emitting the diagnostic twice: Clang doesn't have general code to eliminate redundant diagnostics, it just relies on not doing redundant checks.  So we know in `PerformImplicitConversion` that we're in an arbitrary conversion to `bool`, and the other (original) call site is from code that's specific to the expression-as-condition path.

There's another reason to distinguish between these cases, though: I'm not sure adding parentheses makes as much sense as a way to suppress the warning when the context is an implicit conversion instead of a condition.  The right suppression is to explicitly cast to `bool`.  So (1) we should look through parentheses for implicit conversions and (2) we should emit different notes.


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

https://reviews.llvm.org/D58878