This seems eminently reasonable to warn about. I think people might legitimately complain about extending an existing warning to a new set of contexts without putting it under a new warning flag, though. How annoying would it be to clone the warnings and put them in a subgroup? You could also change the diagnostics to say "converting the result of an assignment to bool" instead of "using the result of an assignment as a condition".
|1 ↗||(On Diff #189061)|
This test should go in test/SemaCXX. You should add a similar test in test/Sema that's C-only and which uses _Bool.
"assignment", in both places
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.