Page MenuHomePhabricator

Remove C++ default arg side table for MS ABI ctor closures

Authored by rnk on Nov 22 2016, 3:32 PM.



We don't need a side table in ASTContext to hold CXXDefaultArgExprs. The
important part of building the CXXDefaultArgExprs was to ODR use the
default argument expressions, not to make AST nodes. Refactor the code
to only check the default argument, and remove the side table in
ASTContext which wasn't being serialized.

Fixes PR31121

Diff Detail


Event Timeline

rnk updated this revision to Diff 78965.Nov 22 2016, 3:32 PM
rnk retitled this revision from to Remove C++ default arg side table for MS ABI ctor closures.
rnk updated this object.
rnk added reviewers: thakis, rsmith, majnemer.
rnk added a subscriber: cfe-commits.
rnk added a comment.Nov 22 2016, 3:39 PM

It's worth pointing out that we have a few more side tables right next door in ASTContext, addCopyConstructorForExceptionObject / getCopyConstructorForExceptionObject. It looks like if those aren't populated, they we might copy the bytes of an exception when catching something by value.

rsmith accepted this revision.Nov 22 2016, 3:58 PM
rsmith edited edge metadata.
rsmith added inline comments.
861 ↗(On Diff #78965)

It would seem a lot more reasonable to actually form an expression to perform the copy of the exception object, and attach that to the ThrowExpr somehow.

Most of this work should not be MS-ABI-specific -- we're required to check that a thrown exception object can be copied (from a non-cv-qualified lvalue) regardless of ABI mode (see [except.throw]/5). In fact, for AST consumers other than IR generation, we could argue that /none/ of this should be MS-ABI specific; those consumers might want to find the implied call to the exception type's copy constructor within the guts of the throw expression, even if we never happen to use it when targeting Itanium.

So... how about we just unconditionally store a copy expression on the ThrowExpr? (With an OpaqueValueExpr representing the exception itself.)

This revision is now accepted and ready to land.Nov 22 2016, 3:58 PM
This revision was automatically updated to reflect the committed changes.
rnk added inline comments.Nov 23 2016, 9:05 AM
861 ↗(On Diff #78965)

Sounds like a good plan. I'm going to take a shot at that ArrayRef on FunctionDecl thing first, though.