[PATCH] D92016: [ASTImporter] Make the Import() return value consistent with the map of imported decls when merging ClassTemplateSpecializationDecls

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Great catch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92016/new/

https://reviews.llvm.org/D92016

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D92016: [ASTImporter] Make the Import() return value consistent with the map of imported decls when merging ClassTemplateSpecializationDecls

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0c926e6d245b: [ASTImporter] Make the Import() return value 
consistent with the map of… (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92016/new/

https://reviews.llvm.org/D92016

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -3104,6 +3104,25 @@
   EXPECT_TRUE(ToFun->hasBody());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, MergeTemplateSpecWithForwardDecl) {
+  std::string ClassTemplate =
+  R"(
+  template
+  struct X { int m; };
+  template<>
+  struct X { int m; };
+  )";
+  // Append a forward decl for our template specialization.
+  getToTuDecl(ClassTemplate + "template<> struct X;", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate, Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X"), isDefinition()));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  // Check that our definition got merged with the existing definition.
+  EXPECT_TRUE(FromSpec->isThisDeclarationADefinition());
+  EXPECT_TRUE(ImportedSpec->isThisDeclarationADefinition());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
   std::string ClassTemplate =
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5423,8 +5423,9 @@
 
   if (PrevDecl) {
 if (IsStructuralMatch(D, PrevDecl)) {
-  if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
-Importer.MapImported(D, PrevDecl->getDefinition());
+  CXXRecordDecl *PrevDefinition = PrevDecl->getDefinition();
+  if (D->isThisDeclarationADefinition() && PrevDefinition) {
+Importer.MapImported(D, PrevDefinition);
 // Import those default field initializers which have been
 // instantiated in the "From" context, but not in the "To" context.
 for (auto *FromField : D->fields()) {
@@ -5446,7 +5447,7 @@
 //
 // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
 // what else could be fused during an AST merge.
-return PrevDecl;
+return PrevDefinition;
   }
 } else { // ODR violation.
   // FIXME HandleNameConflict


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -3104,6 +3104,25 @@
   EXPECT_TRUE(ToFun->hasBody());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, MergeTemplateSpecWithForwardDecl) {
+  std::string ClassTemplate =
+  R"(
+  template
+  struct X { int m; };
+  template<>
+  struct X { int m; };
+  )";
+  // Append a forward decl for our template specialization.
+  getToTuDecl(ClassTemplate + "template<> struct X;", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate, Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X"), isDefinition()));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  // Check that our definition got merged with the existing definition.
+  EXPECT_TRUE(FromSpec->isThisDeclarationADefinition());
+  EXPECT_TRUE(ImportedSpec->isThisDeclarationADefinition());
+}
+
 TEST_P(ASTImporterOptionSpecificTestBase,
ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
   std::string ClassTemplate =
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -5423,8 +5423,9 @@
 
   if (PrevDecl) {
 if (IsStructuralMatch(D, PrevDecl)) {
-  if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
-Importer.MapImported(D, PrevDecl->getDefinition());
+  CXXRecordDecl *PrevDefinition = PrevDecl->getDefinition();
+  if (D->isThisDeclarationADefinition() && PrevDefinition) {
+Importer.MapImported(D, PrevDefinition);
 // Import those default field initializers which have been
 // instantiated in the "From" context, but not in the "To" context.
 for (auto *FromField : D->fields()) {
@@ -5446,7 +5447,7 @@
 //
 // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
 // what else could be fused during an AST merge.
-return PrevDecl;
+return PrevDefinition;
   }
 } else { // ODR