[clang][ASTImporter] Improve import of friend class templates. #74627
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A friend template that is in a dependent context is not linked into declaration chains (for example with the definition of the befriended template). This condition was not correctly handled by ASTImporter.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Balázs Kéri (balazske) ChangesA friend template that is in a dependent context is not linked into declaration chains (for example with the definition of the befriended template). This condition was not correctly handled by Full diff: https://github.com/llvm/llvm-project/pull/74627.diff 2 Files Affected:
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index f1f335118f37a..a243bf65cca27 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -5919,15 +5919,26 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
if (ToD)
return ToD;
- bool IsFriendTemplate = D->getFriendObjectKind() != Decl::FOK_None;
- bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+ // Should check if a declaration is friend in a dependent context.
+ // Such templates are not linked together in a declaration chain.
+ // The ASTImporter strategy is to map existing forward declarations to
+ // imported ones only if strictly necessary, otherwise import these as new
+ // forward declarations. In case of the "dependent friend" declarations, new
+ // declarations are created, but not linked in a declaration chain.
+ auto IsDependentFriend = [](ClassTemplateDecl *TD) {
+ bool IsFriendTemplate = TD->getFriendObjectKind() != Decl::FOK_None;
+ DeclContext *DC = TD->getDeclContext();
+ DeclContext *LexicalDC = TD->getLexicalDeclContext();
+ bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
: DC->isDependentContext();
- bool DependentFriend = IsFriendTemplate && IsDependentContext;
+ return IsFriendTemplate && IsDependentContext;
+ };
+ bool DependentFriend = IsDependentFriend(D);
ClassTemplateDecl *FoundByLookup = nullptr;
// We may already have a template of the same name; try to find and match it.
- if (!DependentFriend && !DC->isFunctionOrMethod()) {
+ if (!DC->isFunctionOrMethod()) {
SmallVector<NamedDecl *, 4> ConflictingDecls;
auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
for (auto *FoundDecl : FoundDecls) {
@@ -5943,10 +5954,13 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
// FIXME: sufficient conditon for 'IgnoreTemplateParmDepth'?
bool IgnoreTemplateParmDepth =
- FoundTemplate->getFriendObjectKind() != Decl::FOK_None &&
- !D->specializations().empty();
+ (FoundTemplate->getFriendObjectKind() != Decl::FOK_None) !=
+ (D->getFriendObjectKind() != Decl::FOK_None);
if (IsStructuralMatch(D, FoundTemplate, /*Complain=*/true,
IgnoreTemplateParmDepth)) {
+ if (DependentFriend || IsDependentFriend(FoundTemplate))
+ continue;
+
ClassTemplateDecl *TemplateWithDef =
getTemplateDefinition(FoundTemplate);
if (D->isThisDeclarationADefinition() && TemplateWithDef)
diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp
index 4dd7510bf8ddf..f9ee73626c948 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -4528,6 +4528,168 @@ TEST_P(ImportFriendClasses, ImportOfClassDefinitionAndFwdFriendShouldBeLinked) {
EXPECT_EQ(ImportedFwd, ImportedDef->getPreviousDecl());
}
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_DefToFriend) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03);
+ auto *ToYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1>
+ struct Y {};
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYDef = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYDef = Import(FromYDef, Lang_CXX03);
+ EXPECT_TRUE(ImportedYDef);
+ EXPECT_FALSE(ImportedYDef->getPreviousDecl());
+ EXPECT_NE(ImportedYDef, ToYFriend);
+}
+
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_DefToFriend_NE) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03);
+ auto *ToYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1, class T2>
+ struct Y {};
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYDef = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYDef = Import(FromYDef, Lang_CXX03);
+ EXPECT_FALSE(ImportedYDef);
+}
+
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_FriendToFriend) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03);
+ auto *ToYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYFriend = Import(FromYFriend, Lang_CXX03);
+ EXPECT_TRUE(ImportedYFriend);
+ EXPECT_FALSE(ImportedYFriend->getPreviousDecl());
+ EXPECT_NE(ImportedYFriend, ToYFriend);
+}
+
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_FriendToFriend_NE) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03);
+ auto *ToYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2, class T3>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYFriend = Import(FromYFriend, Lang_CXX03);
+ EXPECT_FALSE(ImportedYFriend);
+}
+
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_FriendToDef) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct Y {};
+ )",
+ Lang_CXX03);
+ auto *ToYDef = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYFriend = Import(FromYFriend, Lang_CXX03);
+ EXPECT_TRUE(ImportedYFriend);
+ EXPECT_FALSE(ImportedYFriend->getPreviousDecl());
+ EXPECT_NE(ImportedYFriend, ToYDef);
+}
+
+TEST_P(ImportFriendClasses,
+ ImportFriendTemplatesInDependentContext_FriendToDef_NE) {
+ Decl *ToTU = getToTuDecl(
+ R"(
+ template<class T1>
+ struct Y {};
+ )",
+ Lang_CXX03);
+ auto *ToYDef = FirstDeclMatcher<ClassTemplateDecl>().match(
+ ToTU, classTemplateDecl(hasName("Y")));
+ Decl *FromTU = getTuDecl(
+ R"(
+ template<class T1>
+ struct X {
+ template<class T2, class T3>
+ friend struct Y;
+ };
+ )",
+ Lang_CXX03, "input0.cc");
+ auto *FromYFriend = FirstDeclMatcher<ClassTemplateDecl>().match(
+ FromTU, classTemplateDecl(hasName("Y")));
+ auto *ImportedYFriend = Import(FromYFriend, Lang_CXX03);
+ EXPECT_FALSE(ImportedYFriend);
+}
+
TEST_P(ImportFriendClasses, ImportOfRepeatedFriendType) {
const char *Code =
R"(
|
balazske
commented
Dec 6, 2023
| @@ -5943,10 +5954,13 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) { | |||
|
|
|||
| // FIXME: sufficient conditon for 'IgnoreTemplateParmDepth'? | |||
| bool IgnoreTemplateParmDepth = | |||
There was a problem hiding this comment.
Change of this condition was needed to make a failing test ImportOfRecursiveFriendClassTemplateWithNonTypeParm to pass.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
NagyDonat
reviewed
Jan 9, 2024
clang/lib/AST/ASTImporter.cpp
Outdated
Comment on lines
5930
to
5933
| DeclContext *DC = TD->getDeclContext(); | ||
| DeclContext *LexicalDC = TD->getLexicalDeclContext(); | ||
| bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext() | ||
| : DC->isDependentContext(); |
There was a problem hiding this comment.
Suggested change
| DeclContext *DC = TD->getDeclContext(); | |
| DeclContext *LexicalDC = TD->getLexicalDeclContext(); | |
| bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext() | |
| : DC->isDependentContext(); | |
| bool IsDependentContext = TD->getLexicalDeclContext()->isDependentContext(); |
The ternary operator (that you inherited from old code) is superfluous, because if DC != LexicalDC is false, then the two branches are identical.
(By the way, is the logic correct? Perhaps the original author intended to have a difference between the branches...)
There was a problem hiding this comment.
Interesting observation, I think the simplification can be made in the original code too, I did not find this when I reviewed this code.
NagyDonat
approved these changes
Jan 9, 2024
justinfargnoli
pushed a commit
to justinfargnoli/llvm-project
that referenced
this pull request
Jan 28, 2024
…74627) A friend template that is in a dependent context is not linked into declaration chains (for example with the definition of the befriended template). This condition was not correctly handled by `ASTImporter`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A friend template that is in a dependent context is not linked into declaration chains (for example with the definition of the befriended template). This condition was not correctly handled by
ASTImporter.