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

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 12 15:35:23 PST 2018


rjmccall added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:3441
+                                  Value);
+    }
     APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
----------------
cpplearner wrote:
> rjmccall wrote:
> > 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.
> 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.
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.


================
Comment at: clang/lib/AST/ExprConstant.cpp:3453
                                   Value) &&
            handleFloatFloatBinOp(Info, E, Value, Opcode, RHS.getFloat()) &&
            HandleFloatToFloatCast(Info, E, PromotedLHSType, SubobjType, Value);
----------------
rsmith wrote:
> Does this work for the float += int case?
IIRC, the RHS gets promoted to the computation type in Sema.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55413





More information about the cfe-commits mailing list