Implicit conversion from float->bool

[aaron.ballman]

Summary

When performing an implicit from float to bool, the floating point value must be *exactly* zero in order for the conversion to result in 0. This does not involve a conversion through an integer value, and so truncation of the value is not performed. This patch addresses this by using the floating-point value when determining whether to print "true" or "false" in the diagnostic. The behavior of the codegen has always been correct, so this only modifies the wording of the diagnostic to be accurate.

This patch address PR25876.

[Eugene.Zelenko]

This seems to me as specific case of Clang-tidy readability-implicit-bool-cast. May be this check should be entirely moved to Clang?

[aaron.ballman]

In D15814#317879, @Eugene.Zelenko wrote:

This seems to me as specific case of Clang-tidy readability-implicit-bool-cast. May be this check should be entirely moved to Clang?

I think the reason why that is in clang-tidy is because it would be highly chatty and based on user's style preferences. This particular diagnostic has to do with implicit conversions that involve literals requiring conversion to the destination type, which are far less frequent than implicit conversions in general.

[rsmith]

test/SemaCXX/warn-literal-conversion.cpp

49–50

What about

bool b5 = 1.0;
bool b6 = 2.0;

? Arguably any float -> bool conversion changes the value (because true and false are not values of type float), so it wouldn't be completely unreasonable to warn even if the literal is 0.0.

[aaron.ballman]

Thanks! I've commit in r256643.

test/SemaCXX/warn-literal-conversion.cpp

49–50

Except those conversions won't cause confusion to the user, so I'm not certain what we gain by diagnosing. Given that some mental models expect 0.99 to convert to 0, which converts to false (because bool is an integral type, so it "must" do the usual integral truncation dance), it makes sense to tell the user "no no no, that converts to true." I'm less convinced about the utility of warning on things like `bool b = 1.99f' where it changes the value from 1.99 to true. Perhaps this should be changed to only diagnose when converting through an integer would result in a different value that converting through the float?

lib/Sema/SemaChecking.cpp

 
   SmallString<16> PrettyTargetValue;
   if (T->isSpecificBuiltinType(BuiltinType::Bool))
-    PrettyTargetValue = IntegerValue == 0 ? "false" : "true";
+    PrettyTargetValue = Value.isZero() ? "false" : "true";
   else
     IntegerValue.toString(PrettyTargetValue);
 

test/SemaCXX/warn-literal-conversion.cpp

   int y = (24*60*60) * 0.25;
   int pennies = 123.45 * 100;
 }
+
+// Similarly, test floating point conversion to bool. Only float values of zero
+// are converted to false; everything else is converted to true.
+void test1() {
+  bool b1 = 0.99f; // expected-warning {{implicit conversion from 'float' to 'bool' changes value from 0.99 to true}}
+  bool b2 = 0.99; // expected-warning {{implicit conversion from 'double' to 'bool' changes value from 0.99 to true}}
+  // These do not warn because they can be directly converted to integral
+  // values.
+  bool b3 = 0.0f;
+  bool b4 = 0.0;
+}