martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
We fail to import a `ClassTemplateDecl` if the "To" context already contains a
definition and then a forward decl.
This is because `localUncachedLookup` does not find the definition.
This is not a lookup error, the parser behaves differently than assumed in the
importer code.
A `DeclContext` contains one DenseMap (`LookupPtr`) which maps names to lists.
The list is a special list `StoredDeclsList` which is optimized to have one
element.
During building the initial AST, the parser first adds the definition to the
`DeclContext`.
Then during parsing the second declaration (the forward decl) the parser again
calls `DeclContext::addDecl` but that will not add a new element to the
`StoredDeclsList` rarther it simply overwrites the old element with the most
recent one.
This patch fixes the error by finding the definition in the redecl chain.
Added tests for the same issue with `CXXRecordDecl` and with
`ClassTemplateSpecializationDecl`.
These tests pass and they pass because in `VisitRecordDecl` and in
`VisitClassTemplateSpecializationDecl` we already use `D->getDefinition()`
after the lookup.
Repository:
rC Clang
https://reviews.llvm.org/D46950
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/DeclMatcher.h
Index: unittests/AST/DeclMatcher.h
===================================================================
--- unittests/AST/DeclMatcher.h
+++ unittests/AST/DeclMatcher.h
@@ -44,24 +44,35 @@
using FirstDeclMatcher = DeclMatcher<NodeType, DeclMatcherKind::First>;
template <typename NodeType>
-class DeclCounter : public MatchFinder::MatchCallback {
- unsigned Count = 0;
+class DeclCounterWithPredicate : public MatchFinder::MatchCallback {
+ using UnaryPredicate = std::function<bool(const NodeType *)>;
+ UnaryPredicate predicate;
+ unsigned count = 0;
void run(const MatchFinder::MatchResult &Result) override {
- if(Result.Nodes.getNodeAs<NodeType>("")) {
- ++Count;
- }
+ if (auto N = Result.Nodes.getNodeAs<NodeType>("")) {
+ if (predicate(N))
+ ++count;
+ }
}
+
public:
- // Returns the number of matched nodes under the tree rooted in `D`.
+ DeclCounterWithPredicate()
+ : predicate([](const NodeType *) { return true; }) {}
+ DeclCounterWithPredicate(UnaryPredicate p) : predicate(p) {}
+ // Returns the number of matched nodes which satisfy the predicate under the
+ // tree rooted in `D`.
template <typename MatcherType>
unsigned match(const Decl *D, const MatcherType &AMatcher) {
MatchFinder Finder;
Finder.addMatcher(AMatcher.bind(""), this);
Finder.matchAST(D->getASTContext());
- return Count;
+ return count;
}
};
+template <typename NodeType>
+using DeclCounter = DeclCounterWithPredicate<NodeType>;
+
} // end namespace ast_matchers
} // end namespace clang
Index: unittests/AST/ASTImporterTest.cpp
===================================================================
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -281,8 +281,9 @@
return std::make_tuple(*FoundDecls.begin(), Imported);
}
- // Creates a TU decl for the given source code.
- // May be called several times in a given test.
+ // Creates a TU decl for the given source code which can be used as a From
+ // context. May be called several times in a given test (with different file
+ // name).
TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang,
StringRef FileName = "input.cc") {
assert(
@@ -297,6 +298,16 @@
return Tu.TUDecl;
}
+ // Creates the To context with the given source code and returns the TU decl.
+ TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
+ ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+ ToCode = ToSrcCode;
+ assert(!ToAST);
+ ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+
+ return ToAST->getASTContext().getTranslationUnitDecl();
+ }
+
// Import the given Decl into the ToCtx.
// May be called several times in a given test.
// The different instances of the param From may have different ASTContext.
@@ -1464,6 +1475,120 @@
}
}
+TEST_P(ASTImporterTestBase,
+ ImportDefinitionOfClassTemplateIfThereIsAnExistingFwdDeclAndDefinition) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template <typename T>
+ struct B {
+ void f();
+ };
+
+ template <typename T>
+ struct B;
+ )",
+ Lang_CXX);
+ ASSERT_EQ(1u, DeclCounterWithPredicate<ClassTemplateDecl>(
+ [](const ClassTemplateDecl *T) {
+ return T->isThisDeclarationADefinition();
+ })
+ .match(ToTU, classTemplateDecl()));
+
+ Decl *FromTU = getTuDecl(
+ R"(
+ template <typename T>
+ struct B {
+ void f();
+ };
+ )",
+ Lang_CXX, "input1.cc");
+ ClassTemplateDecl *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("B")));
+
+ Import(FromD, Lang_CXX);
+
+ // We should have only one definition.
+ EXPECT_EQ(1u, DeclCounterWithPredicate<ClassTemplateDecl>(
+ [](const ClassTemplateDecl *T) {
+ return T->isThisDeclarationADefinition();
+ })
+ .match(ToTU, classTemplateDecl()));
+}
+
+TEST_P(ASTImporterTestBase,
+ ImportDefinitionOfClassIfThereIsAnExistingFwdDeclAndDefinition) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ struct B {
+ void f();
+ };
+
+ struct B;
+ )",
+ Lang_CXX);
+ ASSERT_EQ(2u, DeclCounter<CXXRecordDecl>().match(
+ ToTU, cxxRecordDecl(hasParent(translationUnitDecl()))));
+
+ Decl *FromTU = getTuDecl(
+ R"(
+ struct B {
+ void f();
+ };
+ )",
+ Lang_CXX, "input1.cc");
+ auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
+ FromTU, cxxRecordDecl(hasName("B")));
+
+ Import(FromD, Lang_CXX);
+
+ EXPECT_EQ(2u, DeclCounter<CXXRecordDecl>().match(
+ ToTU, cxxRecordDecl(hasParent(translationUnitDecl()))));
+}
+
+TEST_P(
+ ASTImporterTestBase,
+ ImportDefinitionOfClassTemplateSpecializationIfThereIsAnExistingFwdDeclAndDefinition) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template <typename T>
+ struct B;
+
+ template <>
+ struct B<int> {};
+
+ template <>
+ struct B<int>;
+ )",
+ Lang_CXX);
+ // We should have only one definition.
+ ASSERT_EQ(1u, DeclCounterWithPredicate<ClassTemplateSpecializationDecl>(
+ [](const ClassTemplateSpecializationDecl *T) {
+ return T->isThisDeclarationADefinition();
+ })
+ .match(ToTU, classTemplateSpecializationDecl()));
+
+ Decl *FromTU = getTuDecl(
+ R"(
+ template <typename T>
+ struct B;
+
+ template <>
+ struct B<int> {};
+ )",
+ Lang_CXX, "input1.cc");
+ auto *FromD = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
+ FromTU, classTemplateSpecializationDecl(hasName("B")));
+
+ Import(FromD, Lang_CXX);
+
+ // We should have only one definition.
+ EXPECT_EQ(1u, DeclCounterWithPredicate<ClassTemplateSpecializationDecl>(
+ [](const ClassTemplateSpecializationDecl *T) {
+ return T->isThisDeclarationADefinition();
+ })
+ .match(ToTU, classTemplateSpecializationDecl()));
+}
+
INSTANTIATE_TEST_CASE_P(
ParameterizedTests, ASTImporterTestBase,
::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
Index: lib/AST/ASTImporter.cpp
===================================================================
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4070,6 +4070,17 @@
TemplateParams);
}
+// Returns the definition for a (forwad) declaration of a ClassTemplateDecl, if
+// it has any definition in the redecl chain.
+static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
+ CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+ if (!ToTemplatedDef)
+ return nullptr;
+ ClassTemplateDecl *TemplateWithDef =
+ ToTemplatedDef->getDescribedClassTemplate();
+ return TemplateWithDef;
+}
+
Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *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
@@ -4084,7 +4095,7 @@
return Importer.Imported(D, ImportedDef);
}
-
+
// Import the major distinguishing characteristics of this class template.
DeclContext *DC, *LexicalDC;
DeclarationName Name;
@@ -4103,34 +4114,39 @@
for (auto *FoundDecl : FoundDecls) {
if (!FoundDecl->isInIdentifierNamespace(Decl::IDNS_Ordinary))
continue;
-
+
Decl *Found = FoundDecl;
if (auto *FoundTemplate = dyn_cast<ClassTemplateDecl>(Found)) {
- if (IsStructuralMatch(D, FoundTemplate)) {
- // The class templates structurally match; call it the same template.
- // We found a forward declaration but the class to be imported has a
- // definition.
- // FIXME Add this forward declaration to the redeclaration chain.
- if (D->isThisDeclarationADefinition() &&
- !FoundTemplate->isThisDeclarationADefinition())
+ // The class to be imported has a definition.
+ if (D->isThisDeclarationADefinition()) {
+ // Lookup will find the fwd decl only if that is more recent than the
+ // definition. So, lets try to get the definition if that is available
+ // in the redecl chain.
+ ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+ if (!TemplateWithDef)
continue;
+ FoundTemplate = TemplateWithDef; // Continue with the definition.
+ }
+
+ if (IsStructuralMatch(D, FoundTemplate)) {
+ // The class templates structurally match; call it the same template.
- Importer.Imported(D->getTemplatedDecl(),
+ Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
- }
+ }
}
-
+
ConflictingDecls.push_back(FoundDecl);
}
-
+
if (!ConflictingDecls.empty()) {
Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
- ConflictingDecls.data(),
+ ConflictingDecls.data(),
ConflictingDecls.size());
}
-
+
if (!Name)
return nullptr;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
