Page MenuHomePhabricator

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

Authored by cpplearner on Dec 6 2018, 11:33 PM.

Diff Detail

Repository
rC Clang

Event Timeline

cpplearner created this revision.Dec 6 2018, 11:33 PM
riccibruno added inline comments.
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 marked an inline comment as done.Dec 11 2018, 8:21 AM
cpplearner added inline comments.
clang/test/SemaCXX/constant-expression-cxx1y.cpp
343

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

cpplearner updated this revision to Diff 177719.EditedDec 11 2018, 8:23 AM

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

cpplearner marked 3 inline comments as done.Dec 11 2018, 8:24 AM
rjmccall added inline comments.Dec 11 2018, 9:38 PM
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 marked an inline comment as done.Dec 12 2018, 6:46 AM
cpplearner added inline comments.
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 added inline comments.Dec 12 2018, 11:21 AM
clang/lib/AST/ExprConstant.cpp
3453

Does this work for the float += int case?

rjmccall added inline comments.Dec 12 2018, 3:35 PM
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 marked an inline comment as done.Dec 13 2018, 1:03 AM
cpplearner added inline comments.
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 added inline comments.Dec 13 2018, 2:21 PM
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 marked an inline comment as done.
cpplearner marked 2 inline comments as done.Dec 14 2018, 9:45 AM
cpplearner added inline comments.
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 accepted this revision.Dec 14 2018, 11:18 AM

Thanks, that looks good to me.

This revision is now accepted and ready to land.Dec 14 2018, 11:18 AM
This revision was automatically updated to reflect the committed changes.