[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5f2157f8fcae: [clang][ASTImporter] Fix friend class template 
import within dependent context (authored by dingfei fd...@feysh.com).

Changed prior to commit:
  https://reviews.llvm.org/D155661?vs=545913=546303#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

Files:
  clang/docs/ReleaseNotes.rst
  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
@@ -3968,8 +3968,58 @@
   EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
 }
 
-
-struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {};
+struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {
+  void testRecursiveFriendClassTemplate(Decl *FromTu) {
+auto *FromD = FirstDeclMatcher().match(
+FromTu, classTemplateDecl());
+
+auto Pattern = classTemplateDecl(
+has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
+ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
+
+auto *FromFriend =
+FirstDeclMatcher().match(FromD, friendDecl());
+auto *FromRecordOfFriend =
+cast(FromFriend->getFriendDecl())
+->getTemplatedDecl();
+EXPECT_NE(FromRecordOfFriend, FromD->getTemplatedDecl());
+EXPECT_TRUE(FromRecordOfFriend->getPreviousDecl() == nullptr);
+
+auto *FromDC = FromRecordOfFriend->getDeclContext();
+auto *FromLexicalDC = FromRecordOfFriend->getLexicalDeclContext();
+ASSERT_EQ(FromDC, cast(FromTu));
+ASSERT_EQ(FromLexicalDC, cast(FromD->getTemplatedDecl()));
+
+ASSERT_FALSE(FromDC->containsDecl(FromRecordOfFriend));
+ASSERT_FALSE(FromLexicalDC->containsDecl(FromRecordOfFriend));
+ASSERT_FALSE(cast(FromRecordOfFriend)
+ ->getLookupParent()
+ ->lookup(FromRecordOfFriend->getDeclName())
+ .empty());
+
+auto *ToD = Import(FromD, Lang_CXX03);
+EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+
+auto *ToFriend = FirstDeclMatcher().match(ToD, friendDecl());
+auto *ToRecordOfFriend =
+cast(ToFriend->getFriendDecl())->getTemplatedDecl();
+
+EXPECT_NE(ToRecordOfFriend, ToD->getTemplatedDecl());
+EXPECT_TRUE(ToRecordOfFriend->getPreviousDecl() == nullptr);
+
+auto *ToDC = ToRecordOfFriend->getDeclContext();
+auto *ToLexicalDC = ToRecordOfFriend->getLexicalDeclContext();
+ASSERT_EQ(ToDC, cast(ToD->getTranslationUnitDecl()));
+ASSERT_EQ(ToLexicalDC, cast(ToD->getTemplatedDecl()));
+
+ASSERT_FALSE(ToDC->containsDecl(ToRecordOfFriend));
+ASSERT_FALSE(ToLexicalDC->containsDecl(ToRecordOfFriend));
+ASSERT_FALSE(cast(ToRecordOfFriend)
+ ->getLookupParent()
+ ->lookup(ToRecordOfFriend->getDeclName())
+ .empty());
+  }
+};
 
 TEST_P(ImportFriendClasses, ImportOfFriendRecordDoesNotMergeDefinition) {
   Decl *FromTU = getTuDecl(
@@ -4074,20 +4124,19 @@
   )",
   Lang_CXX03, "input.cc");
 
-  auto *FromD =
-  FirstDeclMatcher().match(FromTu, classTemplateDecl());
-  auto *ToD = Import(FromD, Lang_CXX03);
-
-  auto Pattern = classTemplateDecl(
-  has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
-  ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
-  EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+  testRecursiveFriendClassTemplate(FromTu);
+}
 
-  auto *Class =
-  FirstDeclMatcher().match(ToD, classTemplateDecl());
-  auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
-  EXPECT_NE(Friend->getFriendDecl(), Class);
-  EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
+TEST_P(ImportFriendClasses,
+   ImportOfRecursiveFriendClassTemplateWithNonTypeParm) {
+  Decl *FromTu = getTuDecl(
+  R"(
+  template class declToImport {
+template friend class declToImport;
+  };
+  )",
+  Lang_CXX03, "input.cc");
+  testRecursiveFriendClassTemplate(FromTu);
 }
 
 TEST_P(ImportFriendClasses, ProperPrevDeclForClassTemplateDecls) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
 IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod() && 

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Ding Fei via Phabricator via cfe-commits]

danix800 added a comment.

In D155661#4550340 , @balazske wrote:

> The fix looks OK, but the test could be improved and cleaned up (for example 
> `FromClass` is the same as `FromD` in the test, and DeclContext is not 
> checked, can be done like in the test 
> `UndeclaredFriendClassShouldNotBeVisible` but the AST is different).

Testcase will be cleaned up in the final commit.

> Probably there are other similar cases, and there is a related problem shown 
> in D156693  (the fix in that patch is not 
> correct, the solution here is not good for that case, it is possible that the 
> same code as here needs to be changed again or a better fix is found). I am 
> accepting this code but probably will create a new patch to improve and add 
> tests for similar cases (if not done before by somebody else).

Confirmed, but I'll not touch this issue in this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

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

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Balázs Kéri via Phabricator via cfe-commits]

balazske accepted this revision.
balazske added a comment.
This revision is now accepted and ready to land.

The fix looks OK, but the test could be improved and cleaned up (for example 
`FromClass` is the same as `FromD` in the test, and DeclContext is not checked, 
can be done like in the test `UndeclaredFriendClassShouldNotBeVisible` but the 
AST is different). Probably there are other similar cases, and there is a 
related problem shown in D156693  (the fix in 
that patch is not correct, the solution here is not good for that case, it is 
possible that the same code as here needs to be changed again or a better fix 
is found). I am accepting this code but probably will create a new patch to 
improve and add tests for similar cases (if not done before by somebody else).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

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

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 545913.
danix800 added a comment.

Update ReleaseNotes.rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

Files:
  clang/docs/ReleaseNotes.rst
  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
@@ -3968,8 +3968,31 @@
   EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
 }
 
-
-struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {};
+struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {
+  void testRecursiveFriendClassTemplate(Decl *FromTu) {
+auto *FromD = FirstDeclMatcher().match(
+FromTu, classTemplateDecl());
+auto *ToD = Import(FromD, Lang_CXX03);
+
+auto Pattern = classTemplateDecl(
+has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
+ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
+EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+
+auto *FromFriend =
+FirstDeclMatcher().match(FromD, friendDecl());
+auto *FromClass =
+FirstDeclMatcher().match(FromD, classTemplateDecl());
+EXPECT_NE(FromFriend->getFriendDecl(), FromClass);
+EXPECT_TRUE(FromFriend->getFriendDecl()->getPreviousDecl() == nullptr);
+
+auto *Class =
+FirstDeclMatcher().match(ToD, classTemplateDecl());
+auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
+EXPECT_NE(Friend->getFriendDecl(), Class);
+EXPECT_TRUE(Friend->getFriendDecl()->getPreviousDecl() == nullptr);
+  }
+};
 
 TEST_P(ImportFriendClasses, ImportOfFriendRecordDoesNotMergeDefinition) {
   Decl *FromTU = getTuDecl(
@@ -4074,20 +4097,19 @@
   )",
   Lang_CXX03, "input.cc");
 
-  auto *FromD =
-  FirstDeclMatcher().match(FromTu, classTemplateDecl());
-  auto *ToD = Import(FromD, Lang_CXX03);
-
-  auto Pattern = classTemplateDecl(
-  has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
-  ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
-  EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+  testRecursiveFriendClassTemplate(FromTu);
+}
 
-  auto *Class =
-  FirstDeclMatcher().match(ToD, classTemplateDecl());
-  auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
-  EXPECT_NE(Friend->getFriendDecl(), Class);
-  EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
+TEST_P(ImportFriendClasses,
+   ImportOfRecursiveFriendClassTemplateWithNonTypeParm) {
+  Decl *FromTu = getTuDecl(
+  R"(
+  template class declToImport {
+template friend class declToImport;
+  };
+  )",
+  Lang_CXX03, "input.cc");
+  testRecursiveFriendClassTemplate(FromTu);
 }
 
 TEST_P(ImportFriendClasses, ProperPrevDeclForClassTemplateDecls) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
 IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod() && !D->isLambda()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {
 SmallVector ConflictingDecls;
 auto FoundDecls =
 Importer.findDeclsInToCtx(DC, SearchName);
@@ -5796,10 +5800,15 @@
   if (ToD)
 return ToD;
 
+  bool IsFriendTemplate = D->getFriendObjectKind() != Decl::FOK_None;
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   ClassTemplateDecl *FoundByLookup = nullptr;
 
   // We may already have a template of the same name; try to find and match it.
-  if (!DC->isFunctionOrMethod()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
 for (auto *FoundDecl : FoundDecls) {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -131,6 +131,8 @@
 
 Bug Fixes to AST Handling
 ^
+- Fixed an import failure of recursive friend class template.
+  `Issue 64169 `_
 
 Miscellaneous Bug Fixes
 ^^^

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Ding Fei via Phabricator via cfe-commits]

danix800 added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2862-2866
+  bool ShouldAddRedecl = !(IsFriendTemplate && IsDependentContext);
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod() && !D->isLambda()) {

balazske wrote:
> The code seems to work but I was confused by the different conditions used 
> (is it possible that `IsFriendTemplate` and `ShouldAddRedecl` is true at the 
> same time?). I had the observation that if `ShouldAddRedecl` is false we do 
> not need the whole search for previous decl. At a friend template we shall 
> not find a definition, the loop would just find the last declaration (this 
> value is not used). So I have the idea of this code (could not find the 
> "suggest edit" command):
> 
> ```
>   bool DependentFriend = IsFriendTemplate && IsDependentContext;
> 
>   // We may already have a record of the same name; try to find and match it.
>   RecordDecl *PrevDecl = nullptr;
>   if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {
> 
> ```
This is more clear. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

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

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 545912.
danix800 added a comment.

Cleanup as @balazske suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

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
@@ -3968,8 +3968,31 @@
   EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
 }
 
-
-struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {};
+struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {
+  void testRecursiveFriendClassTemplate(Decl *FromTu) {
+auto *FromD = FirstDeclMatcher().match(
+FromTu, classTemplateDecl());
+auto *ToD = Import(FromD, Lang_CXX03);
+
+auto Pattern = classTemplateDecl(
+has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
+ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
+EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+
+auto *FromFriend =
+FirstDeclMatcher().match(FromD, friendDecl());
+auto *FromClass =
+FirstDeclMatcher().match(FromD, classTemplateDecl());
+EXPECT_NE(FromFriend->getFriendDecl(), FromClass);
+EXPECT_TRUE(FromFriend->getFriendDecl()->getPreviousDecl() == nullptr);
+
+auto *Class =
+FirstDeclMatcher().match(ToD, classTemplateDecl());
+auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
+EXPECT_NE(Friend->getFriendDecl(), Class);
+EXPECT_TRUE(Friend->getFriendDecl()->getPreviousDecl() == nullptr);
+  }
+};
 
 TEST_P(ImportFriendClasses, ImportOfFriendRecordDoesNotMergeDefinition) {
   Decl *FromTU = getTuDecl(
@@ -4074,20 +4097,19 @@
   )",
   Lang_CXX03, "input.cc");
 
-  auto *FromD =
-  FirstDeclMatcher().match(FromTu, classTemplateDecl());
-  auto *ToD = Import(FromD, Lang_CXX03);
-
-  auto Pattern = classTemplateDecl(
-  has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
-  ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
-  EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+  testRecursiveFriendClassTemplate(FromTu);
+}
 
-  auto *Class =
-  FirstDeclMatcher().match(ToD, classTemplateDecl());
-  auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
-  EXPECT_NE(Friend->getFriendDecl(), Class);
-  EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
+TEST_P(ImportFriendClasses,
+   ImportOfRecursiveFriendClassTemplateWithNonTypeParm) {
+  Decl *FromTu = getTuDecl(
+  R"(
+  template class declToImport {
+template friend class declToImport;
+  };
+  )",
+  Lang_CXX03, "input.cc");
+  testRecursiveFriendClassTemplate(FromTu);
 }
 
 TEST_P(ImportFriendClasses, ProperPrevDeclForClassTemplateDecls) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2857,9 +2857,13 @@
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
 IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod() && !D->isLambda()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {
 SmallVector ConflictingDecls;
 auto FoundDecls =
 Importer.findDeclsInToCtx(DC, SearchName);
@@ -5796,10 +5800,15 @@
   if (ToD)
 return ToD;
 
+  bool IsFriendTemplate = D->getFriendObjectKind() != Decl::FOK_None;
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool DependentFriend = IsFriendTemplate && IsDependentContext;
+
   ClassTemplateDecl *FoundByLookup = nullptr;
 
   // We may already have a template of the same name; try to find and match it.
-  if (!DC->isFunctionOrMethod()) {
+  if (!DependentFriend && !DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
 for (auto *FoundDecl : FoundDecls) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:2862-2866
+  bool ShouldAddRedecl = !(IsFriendTemplate && IsDependentContext);
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod() && !D->isLambda()) {

The code seems to work but I was confused by the different conditions used (is 
it possible that `IsFriendTemplate` and `ShouldAddRedecl` is true at the same 
time?). I had the observation that if `ShouldAddRedecl` is false we do not need 
the whole search for previous decl. At a friend template we shall not find a 
definition, the loop would just find the last declaration (this value is not 
used). So I have the idea of this code (could not find the "suggest edit" 
command):

```
  bool DependentFriend = IsFriendTemplate && IsDependentContext;

  // We may already have a record of the same name; try to find and match it.
  RecordDecl *PrevDecl = nullptr;
  if (!DependentFriend && !DC->isFunctionOrMethod() && !D->isLambda()) {

```



Comment at: clang/lib/AST/ASTImporter.cpp:2904
 
-if (IsStructuralMatch(D, FoundRecord)) {
+if (IsFriendTemplate || IsStructuralMatch(D, FoundRecord)) {
   RecordDecl *FoundDef = FoundRecord->getDefinition();

This change is not needed if the code above is used.



Comment at: clang/lib/AST/ASTImporter.cpp:2976
+  D2CXX, D, Importer.getToContext(), D->getTagKind(), DC,
+  *BeginLocOrErr, Loc, Name.getAsIdentifierInfo(),
+  ShouldAddRedecl ? cast_or_null(PrevDecl)

This change is not needed if the code above is used.



Comment at: clang/lib/AST/ASTImporter.cpp:5805-5812
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool ShouldAddRedecl = !(IsFriendTemplate && IsDependentContext);
+
   ClassTemplateDecl *FoundByLookup = nullptr;
 
   // We may already have a template of the same name; try to find and match it.

Similar change here:
```
  bool DependentFriend = IsFriendTemplate && IsDependentContext;

  ClassTemplateDecl *FoundByLookup = nullptr;

  // We may already have a template of the same name; try to find and match it.
  if (!DependentFriend && !DC->isFunctionOrMethod()) {
```
`IsFriendTemplate` and `ShouldAddRedecl` is not needed (no changes in the later 
lines).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

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

[PATCH] D155661: [clang][ASTImporter] Fix friend class template import within dependent context

[Ding Fei via Phabricator via cfe-commits]

danix800 updated this revision to Diff 544993.
danix800 retitled this revision from "[ASTImporter] Fix friend class template 
import within dependent context" to "[clang][ASTImporter] Fix friend class 
template import within dependent context".
danix800 added a comment.

Update ReleaseNotes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155661

Files:
  clang/docs/ReleaseNotes.rst
  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
@@ -3968,8 +3968,31 @@
   EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
 }
 
-
-struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {};
+struct ImportFriendClasses : ASTImporterOptionSpecificTestBase {
+  void testRecursiveFriendClassTemplate(Decl *FromTu) {
+auto *FromD = FirstDeclMatcher().match(
+FromTu, classTemplateDecl());
+auto *ToD = Import(FromD, Lang_CXX03);
+
+auto Pattern = classTemplateDecl(
+has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
+ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
+EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+
+auto *FromFriend =
+FirstDeclMatcher().match(FromD, friendDecl());
+auto *FromClass =
+FirstDeclMatcher().match(FromD, classTemplateDecl());
+EXPECT_NE(FromFriend->getFriendDecl(), FromClass);
+EXPECT_TRUE(FromFriend->getFriendDecl()->getPreviousDecl() == nullptr);
+
+auto *Class =
+FirstDeclMatcher().match(ToD, classTemplateDecl());
+auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
+EXPECT_NE(Friend->getFriendDecl(), Class);
+EXPECT_TRUE(Friend->getFriendDecl()->getPreviousDecl() == nullptr);
+  }
+};
 
 TEST_P(ImportFriendClasses, ImportOfFriendRecordDoesNotMergeDefinition) {
   Decl *FromTU = getTuDecl(
@@ -4074,20 +4097,19 @@
   )",
   Lang_CXX03, "input.cc");
 
-  auto *FromD =
-  FirstDeclMatcher().match(FromTu, classTemplateDecl());
-  auto *ToD = Import(FromD, Lang_CXX03);
-
-  auto Pattern = classTemplateDecl(
-  has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
-  ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
-  EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
+  testRecursiveFriendClassTemplate(FromTu);
+}
 
-  auto *Class =
-  FirstDeclMatcher().match(ToD, classTemplateDecl());
-  auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
-  EXPECT_NE(Friend->getFriendDecl(), Class);
-  EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
+TEST_P(ImportFriendClasses,
+   ImportOfRecursiveFriendClassTemplateWithNonTypeParm) {
+  Decl *FromTu = getTuDecl(
+  R"(
+  template class declToImport {
+template friend class declToImport;
+  };
+  )",
+  Lang_CXX03, "input.cc");
+  testRecursiveFriendClassTemplate(FromTu);
 }
 
 TEST_P(ImportFriendClasses, ProperPrevDeclForClassTemplateDecls) {
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -2857,6 +2857,10 @@
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
 IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
+  bool IsDependentContext = DC != LexicalDC ? LexicalDC->isDependentContext()
+: DC->isDependentContext();
+  bool ShouldAddRedecl = !(IsFriendTemplate && IsDependentContext);
+
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod() && !D->isLambda()) {
@@ -2897,7 +2901,7 @@
 if (!hasSameVisibilityContextAndLinkage(FoundRecord, D))
   continue;
 
-if (IsStructuralMatch(D, FoundRecord)) {
+if (IsFriendTemplate || IsStructuralMatch(D, FoundRecord)) {
   RecordDecl *FoundDef = FoundRecord->getDefinition();
   if (D->isThisDeclarationADefinition() && FoundDef) {
 // FIXME: Structural equivalence check should check for same
@@ -2955,7 +2959,7 @@
 return CDeclOrErr.takeError();
   Numbering.ContextDecl = *CDeclOrErr;
   D2CXX->setLambdaNumbering(Numbering);
-   } else if (DCXX->isInjectedClassName()) {
+} else if (DCXX->isInjectedClassName()) {
   // We have to be careful to do a similar dance to the one in
   // Sema::ActOnStartCXXMemberDeclarations
   const bool DelayTypeCreation = true;
@@ -2967,10 +2971,11 @@
   Importer.getToContext().getTypeDeclType(
   D2CXX, dyn_cast(DC));
 } else {
-  if (GetImportedOrCreateDecl(D2CXX, D, Importer.getToContext(),
-  D->getTagKind(), DC, *BeginLocOrErr, Loc,
-