[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349349: [ASTImporter] Fix redecl chain of classes and class 
templates (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53655?vs=175641&id=178450#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D53655

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -300,6 +300,16 @@
 
 
 
+MatcherDecl>indirectFieldDeclMatcherIndirectFieldDecl>...
+Matches indirect field declarations.
+
+Given
+  struct X { struct { int a; }; };
+indirectFieldDecl()
+  matches 'a'.
+
+
+
 MatcherDecl>labelDeclMatcherLabelDecl>...
 Matches a declaration of label.
 
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1158,6 +1158,17 @@
 ///   matches 'm'.
 extern const internal::VariadicDynCastAllOfMatcher fieldDecl;
 
+/// Matches indirect field declarations.
+///
+/// Given
+/// \code
+///   struct X { struct { int a; }; };
+/// \endcode
+/// indirectFieldDecl()
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher
+indirectFieldDecl;
+
 /// Matches function declarations.
 ///
 /// Example matches f
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1463,7 +1463,9 @@
   if (Map) {
 StoredDeclsMap::iterator Pos = Map->find(ND->getDeclName());
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+StoredDeclsList::DeclsTy *Vec = Pos->second.getAsVector();
+if ((Vec && is_contained(*Vec, ND)) || Pos->second.getAsDecl() == ND)
   Pos->second.remove(ND);
   }
 } while (DC->isTransparentContext() && (DC = DC->getParent()));
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -122,6 +122,8 @@
   return getCanonicalForwardRedeclChain(FD);
 if (auto *VD = dyn_cast(D))
   return getCanonicalForwardRedeclChain(VD);
+if (auto *TD = dyn_cast(D))
+  return getCanonicalForwardRedeclChain(TD);
 llvm_unreachable("Bad declaration kind");
   }
 
@@ -2607,10 +2609,9 @@
   return std::move(Err);
 IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
-IDNS |= Decl::IDNS_Ordinary;
+IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
   // We may already have a record of the same name; try to find and match it.
-  RecordDecl *AdoptDecl = nullptr;
   RecordDecl *PrevDecl = nullptr;
   if (!DC->isFunctionOrMethod()) {
 SmallVector ConflictingDecls;
@@ -2643,26 +2644,22 @@
   }
 
   if (auto *FoundRecord = dyn_cast(Found)) {
-if (!SearchName) {
+// Do not emit false positive diagnostic in case of unnamed
+// struct/union and in case of anonymous structs.  Would be false
+// because there may be several anonymous/unnamed structs in a class.
+// E.g. these are both valid:
+//  struct A { // unnamed structs
+//struct { struct A *next; } entry0;
+//struct { struct A *next; } entry1;
+//  };
+//  struct X { struct { int a; }; struct { int b; }; }; // anon structs
+if (!SearchName)
   if (!IsStructuralMatch(D, FoundRecord, false))
 continue;
-} else {
-  if (!IsStructuralMatch(D, FoundRecord)) {
-ConflictingDecls.push_back(FoundDecl);
-continue;
-  }
-}
 
-PrevDecl = FoundRecord;
-
-if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-  if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
-  || (D->isCompleteDefinition() &&
-  D->isAnonymousStructOrUnion()
-== FoundDef->isAnonymousStructOrUnion())) {
-// The record types structurally match, or the "from" translation
-   

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@rsmith Please raise any objections until Dec 14 (or if this deadline is too 
strict). I'd like to commit this next week latest so it can get in still this 
year.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@rsmith ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@aaron.ballman Thanks for your review. I have updated the patch to remove 
`containsInVector`.

> This seems mostly reasonable to me, but @rsmith is more well-versed in this 
> area and may have further comments. You should give him a few days to chime 
> in before committing.

Of course.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 175641.
martong marked 2 inline comments as done.
martong added a comment.

- Remove containsInVector


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655

Files:
  docs/LibASTMatchersReference.html
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong marked 4 inline comments as done.
martong added inline comments.



Comment at: include/clang/AST/DeclContextInternals.h:125-129
+  bool containsInVector(const NamedDecl *D) {
+assert(getAsVector() && "Must have a vector at this point");
+DeclsTy &Vec = *getAsVector();
+return is_contained(Vec, D);
+  }

aaron.ballman wrote:
> Given that this is only called in one place and is effectively a one-liner, 
> I'd get rid of this function entirely and replace the call below.
Good catch, thank you.



Comment at: lib/AST/ASTImporter.cpp:2821
 
-  Importer.MapImported(D, D2);
 

a_sidorin wrote:
> Are these lines removed due to move of MapImported into Create helper?
Yes, exactly.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

This seems mostly reasonable to me, but @rsmith is more well-versed in this 
area and may have further comments. You should give him a few days to chime in 
before committing.




Comment at: include/clang/AST/DeclContextInternals.h:125-129
+  bool containsInVector(const NamedDecl *D) {
+assert(getAsVector() && "Must have a vector at this point");
+DeclsTy &Vec = *getAsVector();
+return is_contained(Vec, D);
+  }

Given that this is only called in one place and is effectively a one-liner, I'd 
get rid of this function entirely and replace the call below.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher
+indirectFieldDecl;

martong wrote:
> aaron.ballman wrote:
> > Be sure to update Registry.cpp and regenerate the AST matcher documentation 
> > by running clang\docs\tools\dump_ast_matchers.py.
> > 
> > This change feels orthogonal to the rest of the patch; perhaps it should be 
> > split out into its own patch?
> > This change feels orthogonal to the rest of the patch; perhaps it should be 
> > split out into its own patch?
> 
> I agree this part could go into a separate patch, but the first use of this 
> new ASTMatcher is in the new unittests of this patch, so I think it fits 
> better to add them here.
Okay, that's fair enough.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 175482.
martong marked 5 inline comments as done.
martong added a comment.

- Address several minor review comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655

Files:
  docs/LibASTMatchersReference.html
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  lib/ASTMatchers/Dynamic/Registry.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+ 

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong marked 20 inline comments as done.
martong added inline comments.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher
+indirectFieldDecl;

aaron.ballman wrote:
> Be sure to update Registry.cpp and regenerate the AST matcher documentation 
> by running clang\docs\tools\dump_ast_matchers.py.
> 
> This change feels orthogonal to the rest of the patch; perhaps it should be 
> split out into its own patch?
> This change feels orthogonal to the rest of the patch; perhaps it should be 
> split out into its own patch?

I agree this part could go into a separate patch, but the first use of this new 
ASTMatcher is in the new unittests of this patch, so I think it fits better to 
add them here.



Comment at: lib/AST/ASTImporter.cpp:2644
 
-PrevDecl = FoundRecord;
-
-if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-  if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
-  || (D->isCompleteDefinition() &&
-  D->isAnonymousStructOrUnion()
-== FoundDef->isAnonymousStructOrUnion())) {
-// The record types structurally match, or the "from" translation
-// unit only had a forward declaration anyway; call it the same
-// function.
+if (IsStructuralMatch(D, FoundRecord)) {
+  RecordDecl *FoundDef = FoundRecord->getDefinition();

a_sidorin wrote:
> IsStructuralMatch check will be repeated if (!SearchName && 
> IsStructuralMatch). Is it expected behaviour?
Yes, this is intentional.
The first check does not emit any diagnostics. However, in the case you mention 
(`if (!SearchName && IsStructuralMatch)`) we have to emit diagnostics, because 
we found a real mismatch.




Comment at: lib/AST/ASTImporter.cpp:2667
   ConflictingDecls.push_back(FoundDecl);
-}
+} // for
 

a_sidorin wrote:
> Szelethus wrote:
> > Hah. We should do this more often.
> Unfortunately, it is a clear sign that we have to simplify the function. It's 
> better to leave a FIXME instead.
I agree. Further, we should simplify all `Visit*Decl` functions where we 
iterate over the lookup results. The whole iteration could be part of a 
subroutine with a name like `FindEquivalentPreviousDecl`. But, I'd do that as a 
separate patch which focuses only on that refactor.



Comment at: lib/AST/ASTImporter.cpp:5064
+if (!ToTemplated->getPreviousDecl()) {
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();

aaron.ballman wrote:
> Do not use `auto` here as the type is not spelled out in the initialization.
Good catch.



Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+  Decl *From, *To;

a_sidorin wrote:
> Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in 
> another patch?
Sorry, `2` is an obsolete postfix. Removed it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/AST/DeclContextInternals.h:128
+DeclsTy &Vec = *getAsVector();
+DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+return I != Vec.end();

a_sidorin wrote:
> `auto I = llvm::find(Vec, D)`?
I'd go with `return llvm::is_contained(Vec, D);`



Comment at: include/clang/AST/DeclContextInternals.h:125
 
+  bool containsInVector(NamedDecl *D) {
+assert(getAsVector());

`const NamedDecl *D`



Comment at: include/clang/AST/DeclContextInternals.h:126
+  bool containsInVector(NamedDecl *D) {
+assert(getAsVector());
+DeclsTy &Vec = *getAsVector();

Please add a string literal to the assert explaining what is being checked here.



Comment at: include/clang/ASTMatchers/ASTMatchers.h:1169
+///   matches 'a'.
+extern const internal::VariadicDynCastAllOfMatcher
+indirectFieldDecl;

Be sure to update Registry.cpp and regenerate the AST matcher documentation by 
running clang\docs\tools\dump_ast_matchers.py.

This change feels orthogonal to the rest of the patch; perhaps it should be 
split out into its own patch?



Comment at: lib/AST/ASTImporter.cpp:4991
 if (IsStructuralMatch(D, FoundTemplate)) {
-  if (!IsFriend) {
-Importer.MapImported(D->getTemplatedDecl(),
- FoundTemplate->getTemplatedDecl());
-return Importer.MapImported(D, FoundTemplate);
+  ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+  if (D->isThisDeclarationADefinition() && TemplateWithDef) {

a_sidorin wrote:
> Space after '*'?
Space before the asterisk. ;-)



Comment at: lib/AST/ASTImporter.cpp:2729
+
+const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+if (LexicalDC != DC && IsFriend) {

Drop the top-level `const` qualifier.



Comment at: lib/AST/ASTImporter.cpp:2730
+const bool IsFriend = D->isInIdentifierNamespace(Decl::IDNS_TagFriend);
+if (LexicalDC != DC && IsFriend) {
+  DC->makeDeclVisibleInContext(D2);

Elide braces; I'd probably sink the `IsFriend` into here since it's not used 
elsewhere.



Comment at: lib/AST/ASTImporter.cpp:5064
+if (!ToTemplated->getPreviousDecl()) {
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();

Do not use `auto` here as the type is not spelled out in the initialization.



Comment at: lib/AST/ASTImporter.cpp:5073
+
+  if (LexicalDC != DC && IsFriend) {
+DC->makeDeclVisibleInContext(D2);

Elide braces.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added subscribers: aaron.ballman, rsmith.
a_sidorin added a comment.

Please note that changes in AST lib will require AST code owners' approval. 
@rsmith, @aaron.ballman could you please take a look?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

The change looks mostly Ok but there are some comments inline.




Comment at: include/clang/AST/DeclContextInternals.h:128
+DeclsTy &Vec = *getAsVector();
+DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D);
+return I != Vec.end();

`auto I = llvm::find(Vec, D)`?



Comment at: lib/AST/ASTImporter.cpp:2633
+// struct/union and in case of anonymous structs.  Would be false
+// becasue there may be several anonymous/unnamed structs in a class.
+// E.g. these are both valid:

because (typo)



Comment at: lib/AST/ASTImporter.cpp:2644
 
-PrevDecl = FoundRecord;
-
-if (RecordDecl *FoundDef = FoundRecord->getDefinition()) {
-  if ((SearchName && !D->isCompleteDefinition() && !IsFriendTemplate)
-  || (D->isCompleteDefinition() &&
-  D->isAnonymousStructOrUnion()
-== FoundDef->isAnonymousStructOrUnion())) {
-// The record types structurally match, or the "from" translation
-// unit only had a forward declaration anyway; call it the same
-// function.
+if (IsStructuralMatch(D, FoundRecord)) {
+  RecordDecl *FoundDef = FoundRecord->getDefinition();

IsStructuralMatch check will be repeated if (!SearchName && IsStructuralMatch). 
Is it expected behaviour?



Comment at: lib/AST/ASTImporter.cpp:2667
   ConflictingDecls.push_back(FoundDecl);
-}
+} // for
 

Szelethus wrote:
> Hah. We should do this more often.
Unfortunately, it is a clear sign that we have to simplify the function. It's 
better to leave a FIXME instead.



Comment at: lib/AST/ASTImporter.cpp:2821
 
-  Importer.MapImported(D, D2);
 

Are these lines removed due to move of MapImported into Create helper?



Comment at: lib/AST/ASTImporter.cpp:4991
 if (IsStructuralMatch(D, FoundTemplate)) {
-  if (!IsFriend) {
-Importer.MapImported(D->getTemplatedDecl(),
- FoundTemplate->getTemplatedDecl());
-return Importer.MapImported(D, FoundTemplate);
+  ClassTemplateDecl* TemplateWithDef = getDefinition(FoundTemplate);
+  if (D->isThisDeclarationADefinition() && TemplateWithDef) {

Space after '*'?



Comment at: unittests/AST/ASTImporterTest.cpp:3794
+
+TEST_P(ImportFriendClasses, DeclsFromFriendsShouldBeInRedeclChains2) {
+  Decl *From, *To;

Will `DeclsFromFriendsShouldBeInRedeclChains` without number appear in another 
patch?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

This will break LLDB, unless https://reviews.llvm.org/D54863 is applied. 
@shafik Could you please take a look on https://reviews.llvm.org/D54863 ?


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 175127.
martong added a comment.

- Use MostRecentDecl when setting PrevDecl


Repository:
  rC Clang

https://reviews.llvm.org/D53655

Files:
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[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.

Do not forget that there is a fix the to use getMostRecentDecl in 
ASTImporter.cpp line 2666 here.


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> I see that now everything is reverted, the "good" things too (change to 
> indirectFieldDecl and a line split)?

Yes, you are completely right, sorry for this mess. I just have updated so the 
good things remain.


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 175091.
martong marked an inline comment as done.
martong added a comment.

- Keep the good changes and use the name 'containsInVector'


Repository:
  rC Clang

https://reviews.llvm.org/D53655

Files:
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, 

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

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

balazske added a comment.

I see that now everything is reverted, the "good" things too (change to 
indirectFieldDecl and a line split)?


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong marked 3 inline comments as done.
martong added inline comments.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

Szelethus wrote:
> balazske wrote:
> > martong wrote:
> > > Szelethus wrote:
> > > > Contained in?
> > > Indeed, `containedInVector` sounds better, so I renamed.
> > For me, `containsInVector` is the better name, or `hasInVector` ("contains" 
> > is already used at other places but not "contained" and it sounds like it 
> > is not contained any more).
> Sorry about the confusion, my inline was only about the comment above it, 
> that it isn't obvious enough that //what// decl is contained in. But after 
> taking a second look, it's very clear that only `Map` is mutated in this 
> context, so I don't insist on any modification :)
Okay, so I just reverted the name change.


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 175058.
martong added a comment.

- Revert "Minor style changes and rename a function"


Repository:
  rC Clang

https://reviews.llvm.org/D53655

Files:
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

balazske wrote:
> martong wrote:
> > Szelethus wrote:
> > > Contained in?
> > Indeed, `containedInVector` sounds better, so I renamed.
> For me, `containsInVector` is the better name, or `hasInVector` ("contains" 
> is already used at other places but not "contained" and it sounds like it is 
> not contained any more).
Sorry about the confusion, my inline was only about the comment above it, that 
it isn't obvious enough that //what// decl is contained in. But after taking a 
second look, it's very clear that only `Map` is mutated in this context, so I 
don't insist on any modification :)


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

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

balazske added inline comments.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

martong wrote:
> Szelethus wrote:
> > Contained in?
> Indeed, `containedInVector` sounds better, so I renamed.
For me, `containsInVector` is the better name, or `hasInVector` ("contains" is 
already used at other places but not "contained" and it sounds like it is not 
contained any more).


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 174893.
martong marked 4 inline comments as done.
martong added a comment.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.

- Minor style changes and rename a function


Repository:
  rC Clang

https://reviews.llvm.org/D53655

Files:
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+  Decl *FromTU0 = getTuDecl(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(
+  R"(
+  struct X { // reversed order
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

Szelethus wrote:
> Contained in?
Indeed, `containedInVector` sounds better, so I renamed.


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

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

balazske added inline comments.



Comment at: lib/AST/ASTImporter.cpp:5054
+if (!ToTemplated->getPreviousDecl()) {
+  auto *PrevTemplated = 
FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
+  if (ToTemplated != PrevTemplated)

This is a long line that should be split.


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I can't add anything meaningful to the conversation, but I spotted some nits, 
so here they are.




Comment at: include/clang/ASTMatchers/ASTMatchers.h:1150
+/// \endcode
+/// fieldDecl()
+///   matches 'a'.

Did you mean to write `indirectFieldDecl()`?



Comment at: lib/AST/ASTImporter.cpp:2667
   ConflictingDecls.push_back(FoundDecl);
-}
+} // for
 

Hah. We should do this more often.



Comment at: lib/AST/DeclBase.cpp:1469
 assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||

Contained in?


Repository:
  rC Clang

https://reviews.llvm.org/D53655



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

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: a_sidorin, balazske.
Herald added subscribers: cfe-commits, Szelethus, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: a.sidorin.

The crux of the issue that is being fixed is that lookup could not find
previous decls of a friend class. The solution involves making the
friend declarations visible in their decl context (i.e. adding them to
the lookup table).
Also, we simplify `VisitRecordDecl` greatly.

This fix involves two other repairs (without these the unittests fail):
(1) We could not handle the addition of injected class types properly
when a redecl chain was involved, now this is fixed.
(2) DeclContext::removeDecl failed if the lookup table in Vector form
did not contain the to be removed element. This caused troubles in
ASTImporter::ImportDeclContext. This is also fixed.


Repository:
  rC Clang

https://reviews.llvm.org/D53655

Files:
  include/clang/AST/DeclContextInternals.h
  include/clang/ASTMatchers/ASTMatchers.h
  lib/AST/ASTImporter.cpp
  lib/AST/DeclBase.cpp
  lib/ASTMatchers/ASTMatchersInternal.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct {
+  int a;
+};
+struct {
+  int b;
+};
+  };
+  )",
+  "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+  auto *B = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("b")));
+  auto *FB = cast(B->chain().front());
+  RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+  ASSERT_NE(RA, RB);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RB, RB));
+  EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+  auto t = makeTuDecls(
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )",
+  R"(
+  struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+  };
+  )",
+  Lang_C);
+
+  auto *TU = get<0>(t);
+  auto *A = FirstDeclMatcher().match(
+  TU, indirectFieldDecl(hasName("a")));
+  auto *FA = cast(A->chain().front());
+  RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+  auto *TU1 = get<1>(t);
+  auto *A1 = FirstDeclMatcher().match(
+  TU1, indirectFieldDecl(hasName("a")));
+  auto *FA1 = cast(A1->chain().front());
+  RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+  RecordDecl *X =
+  FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+  RecordDecl *X1 =
+  FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+  ASSERT_NE(X, X1);
+  EXPECT_FALSE(testStructuralMatch(X, X1));
+
+  ASSERT_NE(RA, RA1);
+  EXPECT_TRUE(testStructuralMatch(RA, RA));
+  EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+  EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
 TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
   auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
+#include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -1809,6 +1810,65 @@
   EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
 }
 
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+  auto *Code =
+  R"(
+  struct X {
+struct { int a; };
+struct { int b; };
+  };
+  )";
+  Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+  auto *X0 =
+  FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+  auto *X1 =
+  FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+  Import(X0, Lang_C);
+  Import(X1, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  // We expect no (ODR) warning during the import.
+  EX