[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
This revision was automatically updated to reflect the committed changes.
Closed by commit rC332338: [ASTImporter] Extend lookup logic in class templates
(authored by a.sidorin, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D46353?vs=146765=146780#toc
Repository:
rC Clang
https://reviews.llvm.org/D46353
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4108,8 +4108,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // 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())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
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
@@ -4108,8 +4108,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // 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())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
INSTANTIATE_TEST_CASE_P(
ParameterizedTests, ASTImporterTestBase,
::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
martong added a comment. Hi Aleksei, Added the FIXME, can you help me with committing this? Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
martong updated this revision to Diff 146765.
martong added a comment.
- Add FIXME
Repository:
rC Clang
https://reviews.llvm.org/D46353
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
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
@@ -4077,8 +4077,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // 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())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
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
@@ -4077,8 +4077,14 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // 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())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
a.sidorin accepted this revision. a.sidorin added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D46353#1086456, @martong wrote: > > should we add this new declaration to the redeclaration chain like we do it > > for RecordDecls? > > I think, on a long term we should. Otherwise we could loose e.g. C++11 > attributes which are attached to the forward declaration only. > However, I'd do that as a separate commit, because that would require some > independent changes and tests, also other decl kinds like > ClassTemplateSepcializationDecl may be affected as well by that. I'm fine with this. But a FIXME would be very appreciated. Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
martong added a comment. > should we add this new declaration to the redeclaration chain like we do it > for RecordDecls? I think, on a long term we should. Otherwise we could loose e.g. C++11 attributes which are attached to the forward declaration only. However, I'd do that as a separate commit, because that would require some independent changes and tests, also other decl kinds like ClassTemplateSepcializationDecl may be affected as well by that. Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
a.sidorin added a comment. Hello Gabor, Thank you for the patch. It looks mostly good, but I have a question: should we add this new declaration to the redeclaration chain like we do it for RecordDecls? Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
martong added a comment. > During import of a class template, lookup may find a forward declaration and > structural match falsely reports equivalency in between a fwd decl and a > definition. This can happen when the class to be imported does not have any data members. Structural equivalency check the data members only (and not any of the member functions). Repository: rC Clang https://reviews.llvm.org/D46353 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates
martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
During import of a class template, lookup may find a forward declaration and
structural match falsely reports equivalency in between a fwd decl and a
definition. This results that some definitions are not imported if we had
imported a fwd decl previously. This patch gives a fix.
Repository:
rC Clang
https://reviews.llvm.org/D46353
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
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
@@ -4077,8 +4077,13 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // We found a forward declaration but the class to be imported has a
+ // definition.
+ if (D->isThisDeclarationADefinition() &&
+ !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
}
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+ }
+
+ {
+Decl *FromTU = getTuDecl(
+R"(
+template
+struct B {
+ void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+ }
+}
+
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
@@ -4077,8 +4077,13 @@
if (auto *FoundTemplate = dyn_cast(Found)) {
if (IsStructuralMatch(D, FoundTemplate)) {
// The class templates structurally match; call it the same template.
- // FIXME: We may be filling in a forward declaration here. Handle
- // this case!
+
+ // We found a forward declaration but the class to be imported has a
+ // definition.
+ if (D->isThisDeclarationADefinition() &&
+ !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
Importer.Imported(D->getTemplatedDecl(),
FoundTemplate->getTemplatedDecl());
return Importer.Imported(D, FoundTemplate);
