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

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

2019-02-14 Thread Phabricator via Phabricator via cfe-commits

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354074: [Sema] Fix-up a -Wfloat-conversion diagnostic 
(authored by epilk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58145?vs=186774=186922#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58145

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


Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/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);
Index: cfe/trunk/test/SemaCXX/warn-float-conversion.cpp
===
--- cfe/trunk/test/SemaCXX/warn-float-conversion.cpp
+++ cfe/trunk/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: cfe/trunk/test/SemaObjC/conversion.m
===
--- cfe/trunk/test/SemaObjC/conversion.m
+++ cfe/trunk/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
+@property double d;
+@end
 
+void use_double_prop(DoubleProp *dp) {
+  double local = 42;
+  dp.d += local; // no warning
+}


Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/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

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

2019-02-14 Thread Erik Pilkington via Phabricator via cfe-commits

erik.pilkington marked an inline comment as done.
erik.pilkington added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:10634
+  // warning.
+  else if (ResultBT->isFloatingPoint() && ResultBT->getKind() < RBT->getKind() 
&&
+   // We don't want to warn for system macro.

rjmccall wrote:
> 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.
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!


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

https://reviews.llvm.org/D58145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-02-14 Thread Nick Desaulniers via Phabricator via cfe-commits

nickdesaulniers added a comment.

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


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

https://reviews.llvm.org/D58145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-02-13 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

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.




Comment at: clang/lib/Sema/SemaChecking.cpp:10634
+  // warning.
+  else if (ResultBT->isFloatingPoint() && ResultBT->getKind() < RBT->getKind() 
&&
+   // We don't want to warn for system macro.

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.


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

https://reviews.llvm.org/D58145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-02-13 Thread 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
+@property double d;
+@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);


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
+@property double d;
+@end

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

2019-02-12 Thread Nick Desaulniers via Phabricator via cfe-commits

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

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`?




Comment at: clang/test/SemaObjC/conversion.m:23
+  double local = 42;
+  dp.d += local;
+}

does this need a `// no-warning` comment?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58145



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2019-02-12 Thread Erik Pilkington via Phabricator via cfe-commits

erik.pilkington created this revision.
erik.pilkington added reviewers: rjmccall, rsmith, nickdesaulniers.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.

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


Repository:
  rC Clang

https://reviews.llvm.org/D58145

Files:
  clang/lib/Sema/SemaChecking.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
+@property double d;
+@end
 
+void use_double_prop(DoubleProp *dp) {
+  double local = 42;
+  dp.d += local;
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10624,16 +10624,15 @@
   // 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);


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
+@property double d;
+@end
 
+void use_double_prop(DoubleProp *dp) {
+  double local = 42;
+  dp.d += local;
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10624,16 +10624,15 @@
   // 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

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

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

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

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

7 matches

Site Navigation

Mail list logo

Footer information