[PATCH] D58145: [Sema] Fix a bogus -Wconversion warning

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington updated this revision to Diff 186774.
erik.pilkington marked 2 inline comments as done.
erik.pilkington added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58145/new/

https://reviews.llvm.org/D58145

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/SemaCXX/warn-float-conversion.cpp
  clang/test/SemaObjC/conversion.m


Index: clang/test/SemaObjC/conversion.m
===================================================================
--- clang/test/SemaObjC/conversion.m
+++ clang/test/SemaObjC/conversion.m
@@ -14,4 +14,11 @@
   x = y; // expected-warning {{implicit conversion loses integer precision: 'int' to 'char'}}
 }
 
+__attribute__((objc_root_class)) @interface DoubleProp
+ at property double d;
+ at end
 
+void use_double_prop(DoubleProp *dp) {
+  double local = 42;
+  dp.d += local; // no warning
+}
Index: clang/test/SemaCXX/warn-float-conversion.cpp
===================================================================
--- clang/test/SemaCXX/warn-float-conversion.cpp
+++ clang/test/SemaCXX/warn-float-conversion.cpp
@@ -44,17 +44,17 @@
 void CompoundAssignment() {
   int x = 3;
 
-  x += 1.234;  //expected-warning{{conversion}}
-  x -= -0.0;  //expected-warning{{conversion}}
-  x *= 1.1f;  //expected-warning{{conversion}}
-  x /= -2.2f;  //expected-warning{{conversion}}
+  x += 1.234; // expected-warning {{implicit conversion turns floating-point number into integer: 'double' to 'int'}}
+  x -= -0.0;  // expected-warning {{implicit conversion turns floating-point number into integer: 'double' to 'int'}}
+  x *= 1.1f;  // expected-warning {{implicit conversion turns floating-point number into integer: 'float' to 'int'}}
+  x /= -2.2f; // expected-warning {{implicit conversion turns floating-point number into integer: 'float' to 'int'}}
 
-  int y = x += 1.4f;  //expected-warning{{conversion}}
+  int y = x += 1.4f; // expected-warning {{implicit conversion turns floating-point number into integer: 'float' to 'int'}}
 
   float z = 1.1f;
   double w = -2.2;
 
-  y += z + w;  //expected-warning{{conversion}}
+  y += z + w; // expected-warning {{implicit conversion turns floating-point number into integer: 'double' to 'int'}}
 }
 
 # 1 "foo.h" 3
Index: clang/lib/Sema/SemaChecking.cpp
===================================================================
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10624,16 +10624,16 @@
   // 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())
+    DiagnoseImpCast(S, E, E->getRHS()->getType(), E->getLHS()->getType(),
+                    E->getExprLoc(), diag::warn_impcast_float_integer);
+  // 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->isFloatingPoint() && 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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D58145.186774.patch
Type: text/x-patch
Size: 3449 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190214/cc34932f/attachment.bin>