Page MenuHomePhabricator

Improve diagnostics for literal conversion to Boolean
AbandonedPublic

Authored by aaron.ballman on Jan 6 2016, 1:17 PM.

Details

Reviewers
dblaikie
rsmith
Summary

This patch improves the literal conversion diagnostics where the target type is a Boolean by handling literals more consistently, and telling the user whether the resulting value will be true or false.

This patch changes the behavior of literal conversions from floating-point literals in two ways: (1) we now always diagnose instead of silently accepting conversions where the float can be converted to an exact integer value, and (2) we drop the information about what the source value of the floating-point literal is (it's unimportant information in the context of the Boolean conversion).

This patch removes the string literal conversion diagnostic (and -Wstring-conversion) which is off by default, and rolls its functionality into -Wliteral-conversion, which is on by default. Since we already handle string literal to bool conversion through logical && to not diagnose, I do not believe this will increase the chattiness of the diagnostic. In fact, turning the diagnostic to on by default caught a few tests that were accidentally using string->bool literal conversions. This does change the wording of the previous diagnostic, but I think the new wording is an improvement.

This patch now diagnoses character literal conversions to Boolean types where they used to be silently accepted.

Diff Detail

Event Timeline

aaron.ballman retitled this revision from to Improve diagnostics for literal conversion to Boolean.
aaron.ballman updated this object.
aaron.ballman added reviewers: rsmith, dblaikie.
aaron.ballman added a subscriber: cfe-commits.

Thanks for working on this. This looks good to me overall, but I am not the expert on Sema :]

Cheers,
Manman

lib/Sema/SemaChecking.cpp
6985

Why do we remove the pretty printing for boolean here? Providing context here will be nice.

aaron.ballman added inline comments.Jan 23 2016, 10:03 AM
lib/Sema/SemaChecking.cpp
6985

Because float->Boolean conversion has moved elsewhere (to line 7207 in the new file).

aaron.ballman marked 2 inline comments as done.Feb 15 2016, 11:48 AM

Ping

Ping. Rebased on ToT.

Post-holiday ping.

rsmith added inline comments.Jan 2 2018, 11:31 AM
include/clang/Basic/DiagnosticSemaKinds.td
3147–3149

Warning groups are cheap, and fine-grained groups give users a way to incrementally roll out our new improved warnings. Can we retain the string-conversion group and add a new warning flag for the new cases (both as subgroups of literal-conversion), rather than rolling them into a single group?

I think this diagnostic text is worse for the string literal case than the old text: the type of the string literal is not relevant to the warning, and displaying it seems confusing. For consistency, something like "implicit conversion from string literal to 'bool'; will always evaluate to 'true'" would work.

Also, please include the word "literal" in the version of this warning for floating-point and character literals. (And actually, maybe using the "implicit conversion from floating-point literal to 'bool' [...]" formulation would be clearer there too, rather than listing a largely-irrelevant type.)

aaron.ballman abandoned this revision.Mar 2 2018, 5:57 AM