Page MenuHomePhabricator

[Diagnostics] Warn for assignments in bool contexts

Authored by xbolva00 on Mar 2 2019, 1:19 PM.

Diff Detail

Event Timeline

xbolva00 created this revision.Mar 2 2019, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2019, 1:19 PM

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.

xbolva00 updated this revision to Diff 189711.Mar 7 2019, 7:01 AM

Addressed notes.

Not sure why we miss
_Bool warn3(int x, _Bool a) {

return x = 0 || a;


in C mode :/

rjmccall added inline comments.Mar 7 2019, 5:53 PM

"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.

xbolva00 marked an inline comment as done.Mar 9 2019, 5:02 AM
xbolva00 added inline comments.

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 :/

rjmccall added inline comments.Mar 9 2019, 5:58 PM

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.

RKSimon resigned from this revision.Mar 21 2019, 8:53 AM
xbolva00 abandoned this revision.Apr 29 2019, 2:07 PM