Page MenuHomePhabricator

[Sema] Use the instantiated name of destructors in FindInstantiatedDecl and RebuildMemberExpr

Authored by ahatanak on Sep 27 2016, 9:15 AM.

Diff Detail


Event Timeline

ahatanak updated this revision to Diff 72664.Sep 27 2016, 9:15 AM
ahatanak retitled this revision from to [Sema] Use the instantiated name of destructors in FindInstantiatedDecl and RebuildMemberExpr .
ahatanak updated this object.
ahatanak added reviewers: doug.gregor, rsmith.
ahatanak added a subscriber: cfe-commits.
rsmith added inline comments.Oct 4 2016, 8:48 AM
4849 ↗(On Diff #72664)

Do we need to do this for conversion function names too? (Eg, operator C1<T>*)

ahatanak added inline comments.Oct 4 2016, 10:27 PM
4849 ↗(On Diff #72664)

I added a definition of "operator C1<T>*" and called it in C1::foo1. I didn't see any crashes or asserts.

Calls to a destructor and a conversion function create different AST nodes (the former creates a MemberExpr and the latter creates a CXXDependentScopeMemberExpr) and are transformed by different functions of TreeTransform.

rsmith added inline comments.Oct 11 2016, 2:44 PM
4849 ↗(On Diff #72664)

Please also check in that testcase.

2127 ↗(On Diff #72664)

I don't see any reason why this should be specific to destructors. But this is the wrong place for this change; by the time we reach Rebuild* we should have already transformed everything that we need to transform.

Perhaps the problem is instead...

11967–11969 ↗(On Diff #72664)

... that we fail to transform DestroyedType here. Can you try fixing this bug and see if that removes the need for your other changes?

ahatanak updated this revision to Diff 74324.Oct 11 2016, 10:23 PM

Added a call to operator C1<T>* in test case.

ahatanak marked an inline comment as done.Oct 11 2016, 10:33 PM

(I'm replying to the comment near TreeTransform.h:11967 because phab doesn't let me hit save)

It looks like RebuildCXXPseudoDestructorExpr isn't even called because a MemberExpr is created instead of a CXXPseudoDestructorExpr.

2127 ↗(On Diff #72664)

If we want to do the transformation before entering RebuildMemberExpr, I think it's also possible to do it in TreeTransform<Derived>::TransformMemberExpr.

ahatanak updated this revision to Diff 76883.Nov 3 2016, 1:57 PM

Rebase. Pass the destructor's DeclarationNameInfo to the call to RebuildMemberExpr rather than getting it inside RebuildMemberExpr.

ahatanak updated this revision to Diff 81172.Dec 12 2016, 6:16 PM

Rebase and ping.

ahatanak updated this revision to Diff 82120.Dec 20 2016, 10:49 AM

Call TransformDeclarationNameInfo unconditionally to transform NameInfo rather than doing so only for destructors. I think this is a cleaner fix.

rsmith edited edge metadata.Dec 20 2016, 11:18 AM

Looks good, other than error recovery.

4851 ↗(On Diff #82120)

You should deal with the possibility of SubstDeclarationNameInfo failing (which will happen if substitution into the name creates an invalid type). If you get a null name back from the Subst call, just return null.

8766–8767 ↗(On Diff #82120)

Likewise here, you should return ExprError() if the transformed name is null (please also add an assert that the name is not null before the transform, as in that case a null transformed name would not indicate an error has been diagnosed).

ahatanak updated this revision to Diff 82134.Dec 20 2016, 12:47 PM
ahatanak edited edge metadata.
ahatanak marked 2 inline comments as done.

Add code for error recovery.

8766–8767 ↗(On Diff #82120)

I discovered that the name can be null when the member expression is a member of an anonymous union and therefore an assert would cause a few regression tests to fail. For example, the following code would assert:

struct T0 {
  union {
    void *m0;
template <typename T>
struct T1 : public T0 {
  void f0() {
    m0 = 0;

struct A : public T0 { };

void f1(T1<A> *S) { S->f0(); }

Richard, does the updated patch look OK?

rsmith accepted this revision.Jan 30 2017, 2:30 PM
This revision is now accepted and ready to land.Jan 30 2017, 2:30 PM
This revision was automatically updated to reflect the committed changes.