[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 ↗

(On Diff #177708)

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 ↗

(On Diff #177708)

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

345 ↗

(On Diff #177708)

same

[cpplearner]

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

343 ↗

(On Diff #177708)

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 ↗

(On Diff #177719)

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 ↗

(On Diff #177719)

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 ↗

(On Diff #177719)

Does this work for the float += int case?

[rjmccall]

clang/lib/AST/ExprConstant.cpp

3441 ↗

(On Diff #177719)

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 ↗

(On Diff #177719)

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

[cpplearner]

clang/lib/AST/ExprConstant.cpp

3441 ↗

(On Diff #177719)

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 ↗

(On Diff #177719)

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 ↗

(On Diff #177719)

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.

lib/AST/ExprConstant.cpp

default:
       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 (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);
   }

test/SemaCXX/constant-expression-cxx1y.cpp

constexpr bool test_int() {
     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;