[AST] Check described template at structural equivalence check.

[balazske]

Summary

When checking a class or function the described class or function template
is checked too.
Improved test with symmetric check, added new tests.

[martong]

Ping.

[martong]

LGTM.

[a_sidorin]

lib/AST/ASTStructuralEquivalence.cpp

1025

I think we can move the changes for both RecordDecl and FunctionDecl into Finish() and use Decl::getDescribedTemplate(). This will both simplify the patch and give us the support for templated VarDecls and TypeAliasDecls for free. What do you think?

[balazske]

lib/AST/ASTStructuralEquivalence.cpp

1025

Yes it can be good to check with getDescribedClassTemplate in Finish. (Similarly, there can be more things that are common to check with all Decl or NamedDecl objects in Finish, specifically the name is checked. Or in some cases the name does not matter, but in others yes?)

[a_sidorin]

lib/AST/ASTStructuralEquivalence.cpp

1025

I think that name always matters for structure equivalence checking. I cannot remember any case where it was false during development of our PoC.

[balazske]

• Rebase, common checks at ASTStructuralEquivalenceContext

[a.sidorin]

Hi Balazs,
I have only two nits. Otherwise, the patch is OK and can be committed without additional approval after the comments are fixed. Thank you!

lib/AST/ASTStructuralEquivalence.cpp

1500

CheckCommonEquivalence/CheckKindSpecificEquivalence?

1643–1644

Equivalent = CommonCheckAtFinish(D1, D2) && SpecialCheckAtFinish(D1, D2))?

[balazske]

• Renamed methods, simplified code, comments updated.

include/clang/AST/ASTStructuralEquivalence.h

struct StructuralEquivalenceContext {
   /// FIXME: This is needed by ASTImporter and ASTStructureEquivalence. It
   /// probably makes more sense in some other common place then here.
   static llvm::Optional<unsigned>
   findUntaggedStructOrUnionIndex(RecordDecl *Anon);
 
 private:
   /// Finish checking all of the structural equivalences.
   ///
-  /// \returns true if an error occurred, false otherwise.
+  /// \returns true if the equivalence check failed (non-equivalence detected),
+  /// false if equivalence was detected.
   bool Finish();
+
+  /// Check for common properties at Finish.
+  /// \returns true if D1 and D2 may be equivalent,
+  /// false if they are for sure not.
+  bool CheckCommonEquivalence(Decl *D1, Decl *D2);
+
+  /// Check for class dependent properties at Finish.
+  /// \returns true if D1 and D2 may be equivalent,
+  /// false if they are for sure not.
+  bool CheckKindSpecificEquivalence(Decl *D1, Decl *D2);
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_ASTSTRUCTURALEQUIVALENCE_H

lib/AST/ASTStructuralEquivalence.cpp

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
 
   // Compare the definitions of these two records. If either or both are
   // incomplete, we assume that they are equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
     return true;
 
   // If any of the records has external storage and we do a minimal check (or
-  // AST import) we assmue they are equivalent. (If we didn't have this
+  // AST import) we assume they are equivalent. (If we didn't have this
   // assumption then `RecordDecl::LoadFieldsFromExternalStorage` could trigger
   // another AST import which in turn would call the structural equivalency
   // check again and finally we'd have an improper result.)
   if (Context.EqKind == StructuralEquivalenceKind::Minimal)
     if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
       return true;
 
   if (auto *D1CXX = dyn_cast<CXXRecordDecl>(D1)) {
bool StructuralEquivalenceContext::IsEquivalent(QualType T1, QualType T2) {
   assert(DeclsToCheck.empty());
   assert(TentativeEquivalences.empty());
   if (!::IsStructurallyEquivalent(*this, T1, T2))
     return false;
 
   return !Finish();
 }
 
-bool StructuralEquivalenceContext::Finish() {
-  while (!DeclsToCheck.empty()) {
-    // Check the next declaration.
-    Decl *D1 = DeclsToCheck.front();
-    DeclsToCheck.pop_front();
+bool StructuralEquivalenceContext::CheckCommonEquivalence(Decl *D1, Decl *D2) {
+  // Check for equivalent described template.
+  TemplateDecl *Template1 = D1->getDescribedTemplate();
+  TemplateDecl *Template2 = D2->getDescribedTemplate();
+  if ((Template1 != nullptr) != (Template2 != nullptr))
+    return false;
+  if (Template1 && !IsStructurallyEquivalent(*this, Template1, Template2))
+    return false;
 
-    Decl *D2 = TentativeEquivalences[D1];
-    assert(D2 && "Unrecorded tentative equivalence?");
+  // FIXME: Move check for identifier names into this function.
 
-    bool Equivalent = true;
+  return true;
+}
 
-    // FIXME: Switch on all declaration kinds. For now, we're just going to
-    // check the obvious ones.
-    if (auto *Record1 = dyn_cast<RecordDecl>(D1)) {
-      if (auto *Record2 = dyn_cast<RecordDecl>(D2)) {
-        // Check for equivalent structure names.
-        IdentifierInfo *Name1 = Record1->getIdentifier();
-        if (!Name1 && Record1->getTypedefNameForAnonDecl())
-          Name1 = Record1->getTypedefNameForAnonDecl()->getIdentifier();
-        IdentifierInfo *Name2 = Record2->getIdentifier();
-        if (!Name2 && Record2->getTypedefNameForAnonDecl())
-          Name2 = Record2->getTypedefNameForAnonDecl()->getIdentifier();
-        if (!::IsStructurallyEquivalent(Name1, Name2) ||
-            !::IsStructurallyEquivalent(*this, Record1, Record2))
-          Equivalent = false;
-      } else {
-        // Record/non-record mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *Enum1 = dyn_cast<EnumDecl>(D1)) {
-      if (auto *Enum2 = dyn_cast<EnumDecl>(D2)) {
-        // Check for equivalent enum names.
-        IdentifierInfo *Name1 = Enum1->getIdentifier();
-        if (!Name1 && Enum1->getTypedefNameForAnonDecl())
-          Name1 = Enum1->getTypedefNameForAnonDecl()->getIdentifier();
-        IdentifierInfo *Name2 = Enum2->getIdentifier();
-        if (!Name2 && Enum2->getTypedefNameForAnonDecl())
-          Name2 = Enum2->getTypedefNameForAnonDecl()->getIdentifier();
-        if (!::IsStructurallyEquivalent(Name1, Name2) ||
-            !::IsStructurallyEquivalent(*this, Enum1, Enum2))
-          Equivalent = false;
-      } else {
-        // Enum/non-enum mismatch
-        Equivalent = false;
-      }
-    } else if (const auto *Typedef1 = dyn_cast<TypedefNameDecl>(D1)) {
-      if (const auto *Typedef2 = dyn_cast<TypedefNameDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(Typedef1->getIdentifier(),
-                                        Typedef2->getIdentifier()) ||
-            !::IsStructurallyEquivalent(*this, Typedef1->getUnderlyingType(),
-                                        Typedef2->getUnderlyingType()))
-          Equivalent = false;
-      } else {
-        // Typedef/non-typedef mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *ClassTemplate1 = dyn_cast<ClassTemplateDecl>(D1)) {
-      if (auto *ClassTemplate2 = dyn_cast<ClassTemplateDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, ClassTemplate1,
-                                        ClassTemplate2))
-          Equivalent = false;
-      } else {
-        // Class template/non-class-template mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *FunctionTemplate1 = dyn_cast<FunctionTemplateDecl>(D1)) {
-      if (auto *FunctionTemplate2 = dyn_cast<FunctionTemplateDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, FunctionTemplate1,
-                                        FunctionTemplate2))
-          Equivalent = false;
-      } else {
-        // Class template/non-class-template mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *TTP1 = dyn_cast<TemplateTypeParmDecl>(D1)) {
-      if (auto *TTP2 = dyn_cast<TemplateTypeParmDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
-          Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *NTTP1 = dyn_cast<NonTypeTemplateParmDecl>(D1)) {
-      if (auto *NTTP2 = dyn_cast<NonTypeTemplateParmDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, NTTP1, NTTP2))
-          Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *TTP1 = dyn_cast<TemplateTemplateParmDecl>(D1)) {
-      if (auto *TTP2 = dyn_cast<TemplateTemplateParmDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
-          Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    } else if (auto *MD1 = dyn_cast<CXXMethodDecl>(D1)) {
-      if (auto *MD2 = dyn_cast<CXXMethodDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, MD1, MD2))
-          Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    } else if (FunctionDecl *FD1 = dyn_cast<FunctionDecl>(D1)) {
-      if (FunctionDecl *FD2 = dyn_cast<FunctionDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(FD1->getIdentifier(),
-                                        FD2->getIdentifier()))
-          Equivalent = false;
-        if (!::IsStructurallyEquivalent(*this, FD1, FD2))
-          Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    } else if (FriendDecl *FrD1 = dyn_cast<FriendDecl>(D1)) {
-      if (FriendDecl *FrD2 = dyn_cast<FriendDecl>(D2)) {
-          if (!::IsStructurallyEquivalent(*this, FrD1, FrD2))
-            Equivalent = false;
-      } else {
-        // Kind mismatch.
-        Equivalent = false;
-      }
-    }
+bool StructuralEquivalenceContext::CheckKindSpecificEquivalence(
+    Decl *D1, Decl *D2) {
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+      return false;
+        return false;
+        return false;
+      return false;
+          return false;
+      return false;
+    }
+
+  return true;
 
+bool StructuralEquivalenceContext::Finish() {
+  while (!DeclsToCheck.empty()) {
+    // Check the next declaration.
+    Decl *D1 = DeclsToCheck.front();
+    DeclsToCheck.pop_front();
+
+    Decl *D2 = TentativeEquivalences[D1];
+    assert(D2 && "Unrecorded tentative equivalence?");
+
+    bool Equivalent =
+        CheckCommonEquivalence(D1, D2) && CheckKindSpecificEquivalence(D1, D2);
+
     if (!Equivalent) {
       // Note that these two declarations are not equivalent (and we already
       // know about it).
       NonEquivalentDecls.insert(
           std::make_pair(D1->getCanonicalDecl(), D2->getCanonicalDecl()));
       return true;
     }
-    // FIXME: Check other declaration kinds!
   }
 
   return false;
 }

unittests/AST/StructuralEquivalenceTest.cpp

struct StructuralEquivalenceTest : ::testing::Test {
   std::tuple<NamedDecl *, NamedDecl *> makeNamedDecls(
       const std::string &SrcCode0, const std::string &SrcCode1,
       Language Lang, const char *const Identifier = "foo") {
     auto Matcher = namedDecl(hasName(Identifier));
     return makeDecls<NamedDecl>(SrcCode0, SrcCode1, Lang, Matcher);
   }
 
   bool testStructuralMatch(Decl *D0, Decl *D1) {
-    llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
-    StructuralEquivalenceContext Ctx(
-        D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
-        StructuralEquivalenceKind::Default, false, false);
-    return Ctx.IsEquivalent(D0, D1);
+    llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls01;
+    llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls10;
+    StructuralEquivalenceContext Ctx01(
+        D0->getASTContext(), D1->getASTContext(),
+        NonEquivalentDecls01, StructuralEquivalenceKind::Default, false, false);
+    StructuralEquivalenceContext Ctx10(
+        D1->getASTContext(), D0->getASTContext(),
+        NonEquivalentDecls10, StructuralEquivalenceKind::Default, false, false);
+    bool Eq01 = Ctx01.IsEquivalent(D0, D1);
+    bool Eq10 = Ctx10.IsEquivalent(D1, D0);
+    EXPECT_EQ(Eq01, Eq10);
+    return Eq01;
   }
 
   bool testStructuralMatch(std::tuple<Decl *, Decl *> t) {
     return testStructuralMatch(get<0>(t), get<1>(t));
   }
 };
 
 TEST_F(StructuralEquivalenceTest, Int) {
TEST_F(StructuralEquivalenceTest, WrongOrderOfFieldsInClass) {
   RD->addDeclInternal(FD);
 
   EXPECT_FALSE(testStructuralMatch(Decls));
 }
 
 struct StructuralEquivalenceFunctionTest : StructuralEquivalenceTest {
 };
 
+TEST_F(StructuralEquivalenceFunctionTest, TemplateVsNonTemplate) {
+  auto t = makeNamedDecls(
+      "void foo();",
+      "template<class T> void foo();",
+      Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceFunctionTest, ParamConstWithRef) {
   auto t = makeNamedDecls("void foo(int&);",
                           "void foo(const int&);", Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceFunctionTest, ParamConstSimple) {
   auto t = makeNamedDecls("void foo(int);",
TEST_F(StructuralEquivalenceRecordTest,
   auto *R1 = getRecordDecl(Entry1);
 
   ASSERT_NE(R0, R1);
   EXPECT_TRUE(testStructuralMatch(R0, R0));
   EXPECT_TRUE(testStructuralMatch(R1, R1));
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, TemplateVsNonTemplate) {
+  auto t = makeDecls<CXXRecordDecl>(
+      "struct A { };",
+      "template<class T> struct A { };",
+      Lang_CXX,
+      cxxRecordDecl(hasName("A")));
+  EXPECT_FALSE(testStructuralMatch(t));
+}
 
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
       "struct A{ }; struct B{ }; void foo(A a, A b);",
       "struct A{ }; struct B{ }; void foo(A a, B b);",
       Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 } // end namespace ast_matchers
 } // end namespace clang