r333082 - Fix duplicate class template definitions problem
Gabor Marton via cfe-commits
cfe-commits at lists.llvm.org
Wed May 23 06:53:36 PDT 2018
Author: martong
Date: Wed May 23 06:53:36 2018
New Revision: 333082
URL: http://llvm.org/viewvc/llvm-project?rev=333082&view=rev
Log:
Fix duplicate class template definitions problem
Summary:
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.
Reviewers: a.sidorin, xazax.hun, szepet
Subscribers: rnkovacs, dkrupp, cfe-commits
Differential Revision: https://reviews.llvm.org/D46950
Modified:
cfe/trunk/lib/AST/ASTImporter.cpp
cfe/trunk/unittests/AST/ASTImporterTest.cpp
cfe/trunk/unittests/AST/DeclMatcher.h
Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=333082&r1=333081&r2=333082&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Wed May 23 06:53:36 2018
@@ -4070,6 +4070,17 @@ ASTNodeImporter::VisitTemplateTemplatePa
TemplateParams);
}
+// Returns the definition for a (forward) 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 @@ Decl *ASTNodeImporter::VisitClassTemplat
return Importer.Imported(D, ImportedDef);
}
-
+
// Import the major distinguishing characteristics of this class template.
DeclContext *DC, *LexicalDC;
DeclarationName Name;
@@ -4103,34 +4114,39 @@ Decl *ASTNodeImporter::VisitClassTemplat
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 is a definition.
+ if (D->isThisDeclarationADefinition()) {
+ // Lookup will find the fwd decl only if that is more recent than the
+ // definition. So, 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.
+ }
- Importer.Imported(D->getTemplatedDecl(),
+ if (IsStructuralMatch(D, FoundTemplate)) {
+ // The class templates structurally match; call it the same template.
+
+ 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;
}
Modified: cfe/trunk/unittests/AST/ASTImporterTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/ASTImporterTest.cpp?rev=333082&r1=333081&r2=333082&view=diff
==============================================================================
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp (original)
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp Wed May 23 06:53:36 2018
@@ -281,8 +281,9 @@ public:
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 @@ public:
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,121 @@ TEST_P(ASTImporterTestBase, ImportDefini
}
}
+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,
+ ImportDefinitionOfClassTemplateSpecIfThereIsAnExistingFwdDeclAndDefinition)
+{
+ 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"}),);
Modified: cfe/trunk/unittests/AST/DeclMatcher.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/AST/DeclMatcher.h?rev=333082&r1=333081&r2=333082&view=diff
==============================================================================
--- cfe/trunk/unittests/AST/DeclMatcher.h (original)
+++ cfe/trunk/unittests/AST/DeclMatcher.h Wed May 23 06:53:36 2018
@@ -44,15 +44,23 @@ template <typename NodeType>
using FirstDeclMatcher = DeclMatcher<NodeType, DeclMatcherKind::First>;
template <typename NodeType>
-class DeclCounter : public MatchFinder::MatchCallback {
+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>("")) {
+ 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;
@@ -62,6 +70,9 @@ public:
}
};
+template <typename NodeType>
+using DeclCounter = DeclCounterWithPredicate<NodeType>;
+
} // end namespace ast_matchers
} // end namespace clang
More information about the cfe-commits
mailing list