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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
