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

[ahatanak]

Summary

This fixes PR30361.

clang was failing to compile the test case because it was passing "~C1<T>" instead of "~C1" to FindInstantiatedDecl and RebuildMemberExpr.

[ahatanak]

ping

[rsmith]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4849 ↗

(On Diff #72664)

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

[ahatanak]

lib/Sema/SemaTemplateInstantiateDecl.cpp

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]

lib/Sema/SemaTemplateInstantiateDecl.cpp

4849 ↗

(On Diff #72664)

Please also check in that testcase.

lib/Sema/TreeTransform.h

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]

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

[ahatanak]

(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.

lib/Sema/TreeTransform.h

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]

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

[ahatanak]

Rebase and ping.

[ahatanak]

ping

[ahatanak]

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

[rsmith]

Looks good, other than error recovery.

lib/Sema/SemaTemplateInstantiateDecl.cpp

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.

lib/Sema/TreeTransform.h

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]

Add code for error recovery.

lib/Sema/TreeTransform.h

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(); }

[ahatanak]

Richard, does the updated patch look OK?

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (CXXRecordDecl *Spec = dyn_cast<CXXRecordDecl>(ParentDC)) {
 
         ParentDC = Tag->getDecl();
       }
     }
 
     NamedDecl *Result = nullptr;
     // FIXME: If the name is a dependent name, this lookup won't necessarily
     // find it. Does that ever matter?
-    if (D->getDeclName()) {
-      DeclContext::lookup_result Found = ParentDC->lookup(D->getDeclName());
+    if (auto Name = D->getDeclName()) {
+      DeclarationNameInfo NameInfo(Name, D->getLocation());
+      Name = SubstDeclarationNameInfo(NameInfo, TemplateArgs).getName();
+      if (!Name)
+        return nullptr;
+      DeclContext::lookup_result Found = ParentDC->lookup(Name);
       Result = findInstantiationOf(Context, D, Found.begin(), Found.end());
     } else {
       // Since we don't have a name for the entity we're looking for,
       // our only option is to walk through all of the declarations to
       // find that name. This will occur in a few cases:
       //
       //   - anonymous struct/union within a template
       //   - unnamed class/struct/union/enum within a template

cfe/trunk/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformMemberExpr(MemberExpr *E) {
   SourceLocation FakeOperatorLoc =
       SemaRef.getLocForEndOfToken(E->getBase()->getSourceRange().getEnd());
 
   // FIXME: to do this check properly, we will need to preserve the
   // first-qualifier-in-scope here, just in case we had a dependent
   // base (and therefore couldn't do the check) and a
   // nested-name-qualifier (and therefore could do the lookup).
   NamedDecl *FirstQualifierInScope = nullptr;
+  DeclarationNameInfo MemberNameInfo = E->getMemberNameInfo();
+  if (MemberNameInfo.getName()) {
+    MemberNameInfo = getDerived().TransformDeclarationNameInfo(MemberNameInfo);
+    if (!MemberNameInfo.getName())
+      return ExprError();
+  }
 
   return getDerived().RebuildMemberExpr(Base.get(), FakeOperatorLoc,
                                         E->isArrow(),
                                         QualifierLoc,
                                         TemplateKWLoc,
-                                        E->getMemberNameInfo(),
+                                        MemberNameInfo,
                                         Member,
                                         FoundDecl,
                                         (E->hasExplicitTemplateArgs()
                                            ? &TransArgs : nullptr),
                                         FirstQualifierInScope);
 }
 
 template<typename Derived>

cfe/trunk/test/SemaCXX/destructor.cpp

 
 class Invalid {
     ~Invalid();
     UnknownType xx; // expected-error{{unknown type name}}
 };
 
 // The constructor definition should not have errors
 Invalid::~Invalid() {}
+
+namespace PR30361 {
+template <typename T>
+struct C1 {
+  ~C1() {}
+  operator C1<T>* () { return nullptr; }
+  void foo1();
+};
+
+template<typename T>
+void C1<T>::foo1() {
+  C1::operator C1<T>*();
+  C1::~C1();
+}
+
+void foo1() {
+  C1<int> x;
+  x.foo1();
+}
+}