[Sema] Fix a bogus -Wconversion warning

[erik.pilkington]

Summary

DiagnoseFloatingImpCast expects an integer type, but the changes introduced in D50467 caused us to warn on any builtin type, even pseudo-object type. This lead to us warning in this property expression's syntactic form. Its not clear to me that we should even be analyzing the syntactic form here, but this patch fixes the regression.

rdar://47644670

Thanks!
Erik

[nickdesaulniers]

Thanks for fixing this regression (sorry, I caused it). Doesn't regress the tests I added. You'll probably want to request to Hans that this gets picked up for the 8.0 release. Not sure if @rsmith has comments on the added fixme?

clang/test/SemaObjC/conversion.m

23

does this need a // no-warning comment?

[erik.pilkington]

I was taking a final look at this, and I noticed that we were sending the wrong arguments to DiagnoseFloatingImpCast. The expression argument is meant to be the source expression with floating-point type, but we were sending in the compound assignment operator, with integral type. Similarly, the type argument is meant to be the (integral) destination type, but we were sending in the floating-point RHS type. This lead to bad diagnostics:

t.c:3:7: warning: implicit conversion turns floating-point number into integer: 'int' to 'double' [-Wfloat-conversion]
    integral_value += double_value;

When really it ought to be 'double' to 'int'. Fix this by just directly calling DiagnoseImpCast, since we don't really need anything DiagnoseFloatingImpCast does anyways.

[rjmccall]

Basically LGTM, especially if we need an emergency fix, but please consider addressing my comment before committing since I'd expect it to be straightforward to solve.

clang/lib/Sema/SemaChecking.cpp

10634

Yeah, can we just make a predicate function for this rank comparison? And really it shouldn't be based on rank: while a difference in FP rank *often* corresponds to a difference in representable range, there are exceptions (notably with long double on non-x86 targets (as well as Win64) which is often equivalent to either double or __float128), and we should probably treat those like we do the integer types, i.e. ignore them.

[nickdesaulniers]

Fix this by just directly calling DiagnoseImpCast, since we don't really need anything DiagnoseFloatingImpCast does anyways.

Good catch; thanks for fixing and improving the tests.

[erik.pilkington]

clang/lib/Sema/SemaChecking.cpp

10634

There is another place that we're doing this in this file, so I put it in a follow up: https://reviews.llvm.org/D58254. Thanks!

clang/lib/Sema/SemaChecking.cpp

   const auto *ResultBT = E->getLHS()->getType()->getAs<BuiltinType>();
   const auto *RBT = cast<CompoundAssignOperator>(E)
                         ->getComputationResultType()
                         ->getAs<BuiltinType>();
 
   // The below checks assume source is floating point.
   if (!ResultBT || !RBT || !RBT->isFloatingPoint()) return;
 
-  // If source is floating point but target is not.
-  if (!ResultBT->isFloatingPoint())
-    return DiagnoseFloatingImpCast(S, E, E->getRHS()->getType(),
-                                   E->getExprLoc());
-
-  // If both source and target are floating points.
-  // Builtin FP kinds are ordered by increasing FP rank.
-  if (ResultBT->getKind() < RBT->getKind() &&
-      // We don't want to warn for system macro.
-      !S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
+  // If source is floating point but target is an integer.
+  if (ResultBT->isInteger())
+    DiagnoseFloatingImpCast(S, E, E->getRHS()->getType(), E->getExprLoc());
+  // If both source and target are floating points. Builtin FP kinds are ordered
+  // by increasing FP rank. FIXME: except _Float16, we currently emit a bogus
+  // warning.
+  else if (ResultBT->isFloatingType() && ResultBT->getKind() < RBT->getKind() &&
+           // We don't want to warn for system macro.
+           !S.SourceMgr.isInSystemMacro(E->getOperatorLoc()))
     // warn about dropping FP rank.
     DiagnoseImpCast(S, E->getRHS(), E->getLHS()->getType(), E->getOperatorLoc(),
                     diag::warn_impcast_float_result_precision);
 }
 
 static std::string PrettyPrintInRange(const llvm::APSInt &Value,
                                       IntRange Range) {
   if (!Range.Width) return "0";

clang/test/SemaObjC/conversion.m

 // RUN: %clang_cc1 -Wconversion -fsyntax-only %s -verify
 
 typedef signed char BOOL;
 __attribute__((objc_root_class)) @interface RDar14415662
 @property (readonly) BOOL stuff;
 @property (readwrite) BOOL otherStuff;
 @end
 
 void radar14415662(RDar14415662 *f, char x, int y) {
   f.otherStuff = !f.stuff; // no-warning
   BOOL b = !f.stuff; // no-warning
 
   // True positive to sanity check warning is working.
   x = y; // expected-warning {{implicit conversion loses integer precision: 'int' to 'char'}}
 }
 
+__attribute__((objc_root_class)) @interface DoubleProp
+@property double d;
+@end
 
+void use_double_prop(DoubleProp *dp) {
+  double local = 42;
+  dp.d += local;
+}