[SEMA] add more -Wfloat-conversion to compound assigment analysis

[nickdesaulniers]

Summary

Fixes Bug: https://bugs.llvm.org/show_bug.cgi?id=27061

[nickdesaulniers]

• rework ordering of conditionals to reduce indentation

[nickdesaulniers]

lib/Sema/SemaChecking.cpp

10413–10416 ↗

(On Diff #159763)

This is essentially the only new code. AnalyzeCompoundAssignment was moved in order to be able to call DiagnoseFloatingImpCast (it's defined above). The rest is a small refactoring of the conditionals.

[pirama]

lib/Sema/SemaChecking.cpp

10411 ↗

(On Diff #159763)

Add a comment explaining this conditional as well?

Return if source and target types are unavailable or if source is not a floating point.

With the comment, it might be cleaner to read if you expand the negation: !ResultBT || !RBT || !RBT->isFloatingPoint()

[nickdesaulniers]

lib/Sema/SemaChecking.cpp

10411 ↗

(On Diff #159763)

ResultBT and RBT are pointers that may be null and need to be checked. The previous code did this before accessing them, I've just moved the checks to be sooner.

If !RBT (if it's nullptr), then !RBT->isFloatingPoint() would be a null deref, so unfortunately I can not expand it as recommended.

[nickdesaulniers]

lib/Sema/SemaChecking.cpp

10411 ↗

(On Diff #159763)

oops, nevermind

[nickdesaulniers]

• clean up conditional and add comment

[acoomans]

LGTM

[aaron.ballman]

lib/Sema/SemaChecking.cpp

10424 ↗

(On Diff #159820)

This looks incorrect to me -- this is testing the rank difference and that it is in a system macro (the ! was dropped). If this didn't cause any tests to fail, we should add a test that would fail for it.

[nickdesaulniers]

lib/Sema/SemaChecking.cpp

10424 ↗

(On Diff #159820)

Thanks for the code review!

Sorry, I don't understand what you mean by "rank difference"? F16 vs F32 vs F64 vs F128 etc?

When you say "this looks incorrect," are you commenting on the code I added (Lines 10415 to 10418) or the prexisting code that I moved (Lines 10420-10427)?

Good catch on dropping the !, that was definitely not intentional (and now I'm curious if dw in vim will delete (! together), will add back.

I'm happy to add a test for the system macro, but I also don't know what that is. Can you explain?

[aaron.ballman]

lib/Sema/SemaChecking.cpp

10424 ↗

(On Diff #159820)

Sorry, my explanation may have been confusing. A better way to phrase it would have been "this isn't doing what the old code used to do, and I don't think you intended to change it." I was talking about the ! that disappeared.

I think you can use linemarker directives to get this to happen:

# 1 "systemheader.h" 1 3
#define MACRO

# 1 "test_file.c" 2
// Your code that uses MACRO.

[nickdesaulniers]

• fix up system macro case and add test coverage for that case.

[nickdesaulniers]

Thanks for the info, I found https://gcc.gnu.org/onlinedocs/cpp/Preprocessor-Output.html helpful. How does this look?

[aaron.ballman]

LGTM!

[nickdesaulniers]

Thank you for the code review.

cfe/trunk/lib/Sema/SemaChecking.cpp

 
 /// Diagnose an implicit cast;  purely a helper for CheckImplicitConversion.
 static void DiagnoseImpCast(Sema &S, Expr *E, QualType T,
                             SourceLocation CContext,
                             unsigned diag, bool pruneControlFlow = false) {
   DiagnoseImpCast(S, E, E->getType(), T, CContext, diag, pruneControlFlow);
 }
 
-/// Analyze the given compound assignment for the possible losing of
-/// floating-point precision.
-static void AnalyzeCompoundAssignment(Sema &S, BinaryOperator *E) {
-  assert(isa<CompoundAssignOperator>(E) &&
-         "Must be compound assignment operation");
-  // Recurse on the LHS and RHS in here
-  AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
-  AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
-
-  // Now check the outermost expression
-  const auto *ResultBT = E->getLHS()->getType()->getAs<BuiltinType>();
-  const auto *RBT = cast<CompoundAssignOperator>(E)
-                        ->getComputationResultType()
-                        ->getAs<BuiltinType>();
-
-  // If both source and target are floating points.
-  if (ResultBT && ResultBT->isFloatingPoint() && RBT && RBT->isFloatingPoint())
-    // Builtin FP kinds are ordered by increasing FP rank.
-    if (ResultBT->getKind() < RBT->getKind())
-      // We don't want to warn for system macro.
-      if (!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);
-}
-
 /// Diagnose an implicit cast from a floating point value to an integer value.
 static void DiagnoseFloatingImpCast(Sema &S, Expr *E, QualType T,
                                     SourceLocation CContext) {
   const bool IsBool = T->isSpecificBuiltinType(BuiltinType::Bool);
   const bool PruneWarnings = S.inTemplateInstantiation();
 
   Expr *InnerE = E->IgnoreParenImpCasts();
   // We also want to warn on, e.g., "int i = -1.234"
S.DiagRuntimeBehavior(E->getExprLoc(), E,
                               << E->getSourceRange() << SourceRange(CContext));
   } else {
     S.Diag(E->getExprLoc(), DiagID)
         << E->getType() << T.getUnqualifiedType() << PrettySourceValue
         << PrettyTargetValue << E->getSourceRange() << SourceRange(CContext);
   }
 }
 
+/// Analyze the given compound assignment for the possible losing of
+/// floating-point precision.
+static void AnalyzeCompoundAssignment(Sema &S, BinaryOperator *E) {
+  assert(isa<CompoundAssignOperator>(E) &&
+         "Must be compound assignment operation");
+  // Recurse on the LHS and RHS in here
+  AnalyzeImplicitConversions(S, E->getLHS(), E->getOperatorLoc());
+  AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
+
+  // Now check the outermost expression
+  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()))
+    // 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";
 
   llvm::APSInt ValueInRange = Value;
   ValueInRange.setIsSigned(!Range.NonNegative);
   ValueInRange = ValueInRange.trunc(Range.Width);
   return ValueInRange.toString(10);

cfe/trunk/test/SemaCXX/warn-float-conversion.cpp

void Convert(float f, double d, long double ld) {
   i = f;  //expected-warning{{conversion}}
   i = d;  //expected-warning{{conversion}}
   i = ld;  //expected-warning{{conversion}}
   l = f;  //expected-warning{{conversion}}
   l = d;  //expected-warning{{conversion}}
   l = ld;  //expected-warning{{conversion}}
 }
 
+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}}
+
+  int y = x += 1.4f;  //expected-warning{{conversion}}
+
+  float z = 1.1f;
+  double w = -2.2;
+
+  y += z + w;  //expected-warning{{conversion}}
+}
+
+# 1 "foo.h" 3
+//          ^ the following text comes from a system header file.
+#define SYSTEM_MACRO_FLOAT(x) do { (x) += 1.1; } while(0)
+# 1 "warn-float-conversion.cpp" 1
+//                              ^ start of a new file.
+void SystemMacro() {
+  float x = 0.0f;
+  SYSTEM_MACRO_FLOAT(x);
+}
+
 void Test() {
   int a1 = 10.0/2.0;  //expected-warning{{conversion}}
   int a2 = 1.0/2.0;  //expected-warning{{conversion}}
   bool a3 = ReturnFloat();  //expected-warning{{conversion}}
   int a4 = 1e30 + 1;  //expected-warning{{conversion}}
 }
 
 void TestConstantFloat() {