[ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

[martong]

Summary

Currently we do not import the implicit CXXRecordDecl of a
ClassTemplateSpecializationDecl. This patch fixes it.

[a.sidorin]

lib/AST/ASTImporter.cpp

1962 ↗

(On Diff #147466)

We are changing import if RecordDecl. Is it possible to add a test that doesn't require templates?
I tried and found that the implicit CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration. That's not true for normal CXXRecordDecls, as I see, so this deserves a comment.

[martong]

• Add new test case for simple CXXRecordDecl
• Add comment about ClassTemplateSpecializationDecl implicit CXXRecordDecl

lib/AST/ASTImporter.cpp

1962 ↗

(On Diff #147466)

Yes, I've added a new test which exercises only a RecordDecl.
Also, added a comment about the specialization re-declaration.

[a.sidorin]

LGTM with a nit.

lib/AST/ASTImporter.cpp

1962 ↗

(On Diff #147994)

Multiline comments are pretty uncommon in LLVM. Could you please replace it with // before commit? I.e.

if (Definition && Definition != D &&
    //
    //
    !D->isImplicit())

[martong]

Remove multiline comment and reorder parts of the condition

cfe/trunk/lib/AST/ASTImporter.cpp

Decl *ASTNodeImporter::VisitEnumDecl(EnumDecl *D) {
   return D2;
 }
 
 Decl *ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
   // If this record has a definition in the translation unit we're coming from,
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+      // In contrast to a normal CXXRecordDecl, the implicit
+      // CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration.
+      // The definition of the implicit CXXRecordDecl in this case is the
+      // ClassTemplateSpecializationDecl itself. Thus, we start with an extra
+      // condition in order to be able to import the implict Decl.
+      !D->isImplicit()) {
     Decl *ImportedDef = Importer.Import(Definition);
     if (!ImportedDef)
       return nullptr;
 
     return Importer.Imported(D, ImportedDef);
   }
-  
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return nullptr;
   if (ToD)

cfe/trunk/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterTestBase,
   EXPECT_TRUE(
       Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"}))));
 }
 
 TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
       R"(
+      struct declToImport {
+      };
+      )",
+      Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier<Decl> Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+      R"(
       template <typename U>
       struct declToImport {
       };
       )",
       Lang_CXX, "", Lang_CXX);
 
   MatchVerifier<Decl> Verifier;
   // Match the implicit Decl.
   auto Matcher = classTemplateDecl(has(cxxRecordDecl(has(cxxRecordDecl()))));
   ASSERT_TRUE(Verifier.match(From, Matcher));
   EXPECT_TRUE(Verifier.match(To, Matcher));
 }
 
 TEST_P(
     ASTImporterTestBase,
-    DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+    ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
       R"(
       template<class T>
       class Base {};
       class declToImport : public Base<declToImport> {};
       )",
       Lang_CXX, "", Lang_CXX);