rjmccall added inline comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:3441
+ Value);
+ }
APSInt LHS = HandleIntToIntCast(Info, E, PromotedLHSType,
----------------
cpplearner wrote:
> rjmccall wrote:
> > 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.
> 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.
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.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55413/new/
https://reviews.llvm.org/D55413
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
