[ExprConstant] Handle compound assignment when LHS has integral type and RHS has floating point type

[cpplearner]

Summary

Fixes PR39858

[riccibruno]

clang/lib/AST/ExprConstant.cpp

3427

Parentheses ? It is always nicer to be able to read this
without having to remember whether || has a higher precedence
than &&.

clang/test/SemaCXX/constant-expression-cxx1y.cpp

343

Why remove a *= 3 instead of just adding a *= 3.1.

345

same

[cpplearner]

clang/test/SemaCXX/constant-expression-cxx1y.cpp

343

Because adding a *= 3.1 here will change the result of the whole calculation?

[cpplearner]

Added parentheses. Restored the original tests and added more tests to the end of this function.

[rjmccall]

clang/lib/AST/ExprConstant.cpp

3441

Can we more fundamentally restructure this entire handler so that, if the compound assignment's computation result type is an arithmetic type, we just promote both operands to that type, do the arithmetic there, and then coerce back down? This is C++, so the LHS type imposes almost no restrictions on the RHS type; also, this is Clang, where we support way too many arithmetic types for our own good.

[cpplearner]

clang/lib/AST/ExprConstant.cpp

3441

It seems the conditional statement is unavoidable, because APSInt and APFloat can't be handled at the same time (i.e. you need to choose among Handle{Int,Float}To{Int,Float}Cast, and between handleIntIntBinOp/handleFloatFloatBinOp). Maybe it's possible to add a layer that can accept both APSInt and APFloat, but it seems like an overkill if it's only used in the compound assignment case.

[rsmith]

clang/lib/AST/ExprConstant.cpp

3453

Does this work for the float += int case?

[rjmccall]

clang/lib/AST/ExprConstant.cpp

3441

But we can have HandleValueTo{Int,Float}Cast functions that start with an arbitrary APValue and do the switch on that type, and we can have a HandleValueValueBinOp function that asserts that its operands have the same type and does the switch, and those two together should be good enough for what we need here.

3453

IIRC, the RHS gets promoted to the computation type in Sema.

[cpplearner]

clang/lib/AST/ExprConstant.cpp

3441

That's what I mean by "add a layer", and I think it's unworthy.

Maybe it makes more sense as part of a larger scale rewrite, but that's more than I can deal with.

[rjmccall]

clang/lib/AST/ExprConstant.cpp

3441

Ah, I see that all three found methods here are called directly. I agree that this makes a rewrite that tries to handle the other cases here harder.

Please at least adjust the code to make the || (!RHS.isInt() && !RHS.isFloat()) check redundant: put the int case in an if (RHS.isInt()) block (which should probably come before the isFloat() block) and emit a diagnostic if none of the cases are triggered.

[cpplearner]

clang/lib/AST/ExprConstant.cpp

3453

Yes, in the float += int case, the RHS is the result of ImplicitCastExpr of kind IntegralToFloating. And test_float() contains test for that case.

[rjmccall]

Thanks, that looks good to me.

clang/lib/AST/ExprConstant.cpp

       Info.FFDiag(E);
       return false;
     }
   }
   bool found(APSInt &Value, QualType SubobjType) {
     if (!checkConst(SubobjType))
       return false;
 
-    if (!SubobjType->isIntegerType() || !RHS.isInt()) {
+    if (!SubobjType->isIntegerType()) {
       // We don't support compound assignment on integer-cast-to-pointer
       // values.
       Info.FFDiag(E);
       return false;
     }
 
-    APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
-                                    SubobjType, Value);
-    if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
-      return false;
-    Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
-    return true;
+    if (RHS.isInt()) {
+      APSInt LHS =
+          HandleIntToIntCast(Info, E, PromotedLHSType, SubobjType, Value);
+      if (!handleIntIntBinOp(Info, E, LHS, Opcode, RHS.getInt(), LHS))
+        return false;
+      Value = HandleIntToIntCast(Info, E, SubobjType, PromotedLHSType, LHS);
+      return true;
+    } else if (RHS.isFloat()) {
+      APFloat FValue(0.0);
+      return HandleIntToFloatCast(Info, E, SubobjType, Value, PromotedLHSType,
+                                  FValue) &&
+             handleFloatFloatBinOp(Info, E, FValue, Opcode, RHS.getFloat()) &&
+             HandleFloatToIntCast(Info, E, PromotedLHSType, FValue, SubobjType,
+                                  Value);
+    }
+
+    Info.FFDiag(E);
+    return false;
   }
   bool found(APFloat &Value, QualType SubobjType) {
     return checkConst(SubobjType) &&
            HandleFloatToFloatCast(Info, E, SubobjType, PromotedLHSType,
                                   Value) &&
            handleFloatFloatBinOp(Info, E, Value, Opcode, RHS.getFloat()) &&
            HandleFloatToFloatCast(Info, E, PromotedLHSType, SubobjType, Value);
   }
   bool foundPointer(APValue &Subobj, QualType SubobjType) {
     if (!checkConst(SubobjType))

clang/test/SemaCXX/constant-expression-cxx1y.cpp

 
 namespace compound_assign {
   constexpr bool test_int() {
     int a = 3;
     a += 6;
     if (a != 9) return false;
     a -= 2;
     if (a != 7) return false;
     a *= 3;
     if (a != 21) return false;
     if (&(a /= 10) != &a) return false;
     if (a != 2) return false;
     a <<= 3;
     if (a != 16) return false;
     a %= 6;
     if (a != 4) return false;
     a >>= 1;
     if (a != 2) return false;
     a ^= 10;
     if (a != 8) return false;
     a |= 5;
     if (a != 13) return false;
     a &= 14;
     if (a != 12) return false;
+    a += -1.2;
+    if (a != 10) return false;
+    a -= 3.1;
+    if (a != 6) return false;
+    a *= 2.2;
+    if (a != 13) return false;
+    if (&(a /= 1.5) != &a) return false;
+    if (a != 8) return false;
     return true;
   }
   static_assert(test_int(), "");
 
   constexpr bool test_float() {
     float f = 123.;
     f *= 2;
     if (f != 246.) return false;