[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 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] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D53818#1307321 , @a_sidorin wrote:

> LGTM. Thank you for addressing my questions!


Hi Alexei,

Could you please also take a look on that patch which this one depends on? 
https://reviews.llvm.org/D53751
I think the changes of that patch are included in this one too. Thank you.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53818



___
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] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 174897.
martong marked an inline comment as done.
martong removed a reviewer: shafik.
martong removed a subscriber: gamesh411.
martong added a comment.
Herald added a reviewer: shafik.

- Better style without braces


Repository:
  rC Clang

https://reviews.llvm.org/D53755

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  lib/AST/ExternalASTMerger.cpp


Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -144,14 +144,14 @@
 }
 if (auto *ToTag = dyn_cast(To)) {
   ToTag->setHasExternalLexicalStorage();
-  ToTag->setMustBuildLookupTable();
+  ToTag->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToTag));
 } else if (auto *ToNamespace = dyn_cast(To)) {
   ToNamespace->setHasExternalVisibleStorage();
   assert(Parent.CanComplete(ToNamespace));
 } else if (auto *ToContainer = dyn_cast(To)) {
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));
 }
 return To;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1580,6 +1580,9 @@
 return Err;
 
   ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+  return Err;
 
   return Error::success();
 }
@@ -7721,17 +7724,12 @@
   return ToAttr;
 }
 
-Decl *ASTImporter::GetAlreadyImportedOrNull(Decl *FromD) {
-  llvm::DenseMap::iterator Pos = ImportedDecls.find(FromD);
-  if (Pos != ImportedDecls.end()) {
-Decl *ToD = Pos->second;
-// FIXME: move this call to ImportDeclParts().
-if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, 
ToD))
-  llvm::consumeError(std::move(Err));
-return ToD;
-  } else {
+Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
+  auto Pos = ImportedDecls.find(FromD);
+  if (Pos != ImportedDecls.end())
+return Pos->second;
+  else
 return nullptr;
-  }
 }
 
 Decl *ASTImporter::Import(Decl *FromD) {
Index: include/clang/AST/ASTImporter.h
===
--- include/clang/AST/ASTImporter.h
+++ include/clang/AST/ASTImporter.h
@@ -198,7 +198,7 @@
 /// Return the copy of the given declaration in the "to" context if
 /// it has already been imported from the "from" context.  Otherwise return
 /// NULL.
-Decl *GetAlreadyImportedOrNull(Decl *FromD);
+Decl *GetAlreadyImportedOrNull(const Decl *FromD) const;
 
 /// Import the given declaration context from the "from"
 /// AST context into the "to" AST context.


Index: lib/AST/ExternalASTMerger.cpp
===
--- lib/AST/ExternalASTMerger.cpp
+++ lib/AST/ExternalASTMerger.cpp
@@ -144,14 +144,14 @@
 }
 if (auto *ToTag = dyn_cast(To)) {
   ToTag->setHasExternalLexicalStorage();
-  ToTag->setMustBuildLookupTable();
+  ToTag->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToTag));
 } else if (auto *ToNamespace = dyn_cast(To)) {
   ToNamespace->setHasExternalVisibleStorage();
   assert(Parent.CanComplete(ToNamespace));
 } else if (auto *ToContainer = dyn_cast(To)) {
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));
 }
 return To;
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1580,6 +1580,9 @@
 return Err;
 
   ToD = cast_or_null(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+  return Err;
 
   return Error::success();
 }
@@ -7721,17 +7724,12 @@
   return ToAttr;
 }
 
-Decl *ASTImporter::GetAlreadyImportedOrNull(Decl *FromD) {
-  llvm::DenseMap::iterator Pos = ImportedDecls.find(FromD);
-  if (Pos != ImportedDecls.end()) {
-Decl *ToD = Pos->second;
-// FIXME: move this call to ImportDeclParts().
-if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD))
-  llvm::consumeError(std::move(Err));
-return ToD;
-  } else {
+Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
+  auto Pos = ImportedDecls.find(FromD);
+  if (Pos != ImportedDecls.end())
+return Pos->second;
+  else
 return nullptr;
-  }
 }
 
 Decl *ASTImporter::Import(Decl *FromD) {
Index: include/clang/AST/ASTImporter.h

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.



Comment at: lib/AST/ASTImporter.cpp:7716
   }
 }
 

balazske wrote:
> This can be simplified by removing brace characters and removing `ToD`.
Good catch, changed it.


Repository:
  rC Clang

https://reviews.llvm.org/D53755



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

[PATCH] D53699: [ASTImporter] Fix inequality of functions with different attributes

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a subscriber: gamesh411.

That's a good point. I agree, we should check some bits (calling convention 
bits) but not all (e.g. noreturn bit). I am going to create another patch which 
replaces this.


Repository:
  rC Clang

https://reviews.llvm.org/D53699



___
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] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

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

- Address Alexei's comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708

Files:
  include/clang/AST/ASTImporter.h
  include/clang/AST/ASTImporterLookupTable.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/AST/ASTImporterLookupTable.cpp
  lib/AST/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/Frontend/ASTMerge.cpp
  tools/clang-import-test/clang-import-test.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -14,6 +14,7 @@
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -307,24 +308,27 @@
   Unit->enableSourceFileDiagnostics();
 }
 
-void lazyInitImporter(ASTUnit *ToAST) {
+void lazyInitImporter(ASTImporterLookupTable , ASTUnit *ToAST) {
   assert(ToAST);
   if (!Importer) {
-Importer.reset(new ASTImporter(
-ToAST->getASTContext(), ToAST->getFileManager(),
-Unit->getASTContext(), Unit->getFileManager(), false));
+Importer.reset(
+new ASTImporter(ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(),
+false, ));
   }
   assert(>getASTContext() == >getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
 
-Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
-  lazyInitImporter(ToAST);
+Decl *import(ASTImporterLookupTable , ASTUnit *ToAST,
+ Decl *FromDecl) {
+  lazyInitImporter(LookupTable, ToAST);
   return Importer->Import(FromDecl);
- }
+}
 
-QualType import(ASTUnit *ToAST, QualType FromType) {
-  lazyInitImporter(ToAST);
+QualType import(ASTImporterLookupTable , ASTUnit *ToAST,
+QualType FromType) {
+  lazyInitImporter(LookupTable, ToAST);
   return Importer->Import(FromType);
 }
   };
@@ -338,13 +342,23 @@
   // vector is expanding, with the list we won't have these issues.
   std::list FromTUs;
 
-  void lazyInitToAST(Language ToLang) {
+  // Initialize the lookup table if not initialized already.
+  void lazyInitLookupTable(TranslationUnitDecl *ToTU) {
+assert(ToTU);
+if (!LookupTablePtr)
+  LookupTablePtr = llvm::make_unique(*ToTU);
+  }
+
+  void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName) {
 if (ToAST)
   return;
 ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+// Source code must be a valid live buffer through the tests lifetime.
+ToCode = ToSrcCode;
 // Build the AST from an empty file.
-ToAST = tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, FileName);
 ToAST->enableSourceFileDiagnostics();
+lazyInitLookupTable(ToAST->getASTContext().getTranslationUnitDecl());
   }
 
   TU *findFromTU(Decl *From) {
@@ -358,6 +372,10 @@
 return &*It;
   }
 
+protected:
+
+  std::unique_ptr LookupTablePtr;
+
 public:
   // We may have several From context but only one To context.
   std::unique_ptr ToAST;
@@ -374,26 +392,23 @@
 FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
 TU  = FromTUs.back();
 
-ToCode = ToSrcCode;
 assert(!ToAST);
-ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
-ToAST->enableSourceFileDiagnostics();
+lazyInitToAST(ToLang, ToSrcCode, OutputFileName);
 
 ASTContext  = FromTU.Unit->getASTContext();
 
-createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-
 IdentifierInfo *ImportedII = (Identifier);
 assert(ImportedII && "Declaration with the given identifier "
  "should be specified in test!");
 DeclarationName ImportDeclName(ImportedII);
-SmallVector FoundDecls;
+SmallVector FoundDecls;
 FromCtx.getTranslationUnitDecl()->localUncachedLookup(ImportDeclName,
   FoundDecls);
 
 assert(FoundDecls.size() == 1);
 
-Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+Decl *Imported =
+FromTU.import(*LookupTablePtr, ToAST.get(), FoundDecls.front());
 
 assert(Imported);
 return std::make_tuple(*FoundDecls.begin(), Imported);
@@ -419,11 +434,8 @@
   // Creates the To context with the given source code and returns the TU decl.
   TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, 

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:7605
+
+ASTImporter::ASTImporter(ASTImporterLookupTable *LookupTable,
+ ASTContext , FileManager ,

a_sidorin wrote:
> It's better to make `LookupTable` an optional parameter of the previous ctor 
> and remove this one.
Okay, I have changed it to be a default initalized to nullptr parameter.



Comment at: lib/AST/ASTImporterLookupTable.cpp:84
+  DeclContext *DC = ND->getDeclContext()->getPrimaryContext();
+  remove(DC, ND);
+  DeclContext *ReDC = DC->getRedeclContext()->getPrimaryContext();

a_sidorin wrote:
> Should we remove DC-related entry if it is empty after deletion?
I don't think so.
The only place where we are going to use `remove` is in case we had an error 
during import (upcoming patch). Even if we had an error before, subsequent 
successfully imported Decls could be part of the same DC (so, chances are we 
would superfluously do a ping-pong of delete/create).



Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+

a_sidorin wrote:
> Could you please explain what does this test do?
Well, it makes sure that we can build a lookup table for an empty file without 
any *assertion*. We may have an assertion in the ctor during traversing the 
AST. I consider this the most basic use case as it tests only the constructor.
However, if you find it weird I can remove it.



Comment at: unittests/AST/ASTImporterTest.cpp:3889
+if (ND->getDeclName() == Name)
+  Found = ND;
+}

a_sidorin wrote:
> Do we need break/early return here?
Yes, that's better to have.



Comment at: unittests/AST/ASTImporterTest.cpp:3930
+
+  auto FindInDeclListOfDC = [](DeclContext *DC, DeclarationName Name) {
+Decl *Found = nullptr;

a_sidorin wrote:
> This lambda is the same as in previous func so it's better to extract it into 
> a helper.
Good catch, thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ExternalASTMerger.cpp:154
   ToContainer->setHasExternalLexicalStorage();
-  ToContainer->setMustBuildLookupTable();
+  ToContainer->getPrimaryContext()->setMustBuildLookupTable();
   assert(Parent.CanComplete(ToContainer));

a_sidorin wrote:
> Do we have to do the same for NamespaceDecls?
Yes, I think so.
The primary context of a `NamespaceDecl` can be other than itself:
```
DeclContext *DeclContext::getPrimaryContext() {
  switch (DeclKind) {
  case Decl::TranslationUnit:
  case Decl::ExternCContext:
  case Decl::LinkageSpec:
  case Decl::Export:
  case Decl::Block:
  case Decl::Captured:
  case Decl::OMPDeclareReduction:
// There is only one DeclContext for these entities.
return this;

  case Decl::Namespace:
// The original namespace is our primary context.
return static_cast(this)->getOriginalNamespace();
```
Consequently, we should call `setMustBuildLookupTable` only on a 
`NamespaceDecl` if we know for sure that is a primary context.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755



___
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] D53693: [ASTImporter] Typedef import brings in the complete type

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.



Comment at: lib/AST/ASTImporter.cpp:2310
+D->getUnderlyingType(), FoundTypedef->getUnderlyingType())) {
+  QualType FromUT = D->getUnderlyingType();
+  QualType FoundUT = FoundTypedef->getUnderlyingType();

a_sidorin wrote:
> We can move these two vars upper and use them in the condition.
Good point, changed it.



Comment at: lib/AST/ASTImporter.cpp:2314
+  // already have a complete underlying type then return with that.
+  if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
 return Importer.MapImported(D, FoundTypedef);

a_sidorin wrote:
> This condition omits the case where both types are complete. Should we use 
> `FromUT->isIncompleteType == FoundUT-isIncompleteType()` instead?
I think you wanted to say "both types are INcomplete".
The condition indeed omits the case where both types are incomplete and that is 
a potential bug, thanks for finding it!
I think the best way to handle this case is to do the something similar what is 
done in case of functions, i.e break the for loop once we know that the found 
and the to be imported Decl are structurally equivalent.
So, I added a `break` to do that and to be similar what we do in case of 
functions.

This is what we have with functions:
```
  if (IsStructuralMatch(D, FoundFunction)) {
const FunctionDecl *Definition = nullptr;
if (D->doesThisDeclarationHaveABody() &&
FoundFunction->hasBody(Definition)) {
  return Importer.MapImported(
  D, const_cast(Definition));
}
FoundByLookup = FoundFunction;
break;
  }
```

I'd like to have something similar with typedefs. The goal is to have only one 
TypedefNameDecl with a complete type. A TypedefNameDecl with a complete type is 
analogous to a FunctionDecl with a definition, we want to have only one in the 
"To" context.
A TypedefNameDecl is also redeclarable, so on a long term we should connect a 
new Decl to the found previous one.




Repository:
  rC Clang

https://reviews.llvm.org/D53693



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

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.

Gentle Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D53708



___
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, 

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Constructs like `struct A { struct Foo *p; };` are very common in C projects. 
Since `Foo` as a forward decl cannot be found by normal lookup we need this 
patch in order to properly CTU analyze even a simple C project like `tmux`.


Repository:
  rC Clang

https://reviews.llvm.org/D53708



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

[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: gamesh411.
martong added a comment.

@shafik , @gamesh411 I think when I used arcanist to update the patch it 
removed you as a reviewer and a subscriber. I am really sorry about this. 
Fortunately the Herald script added you guys back.
And once you are here, this is a gentle ping. :)


Repository:
  rC Clang

https://reviews.llvm.org/D53755



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

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a subscriber: gamesh411.

@shafik,

I could run the libcxx tests on my Linux machine (Ubuntu 16.04) by installing 
libcxx: `sudo apt-get install libc++-dev`.
From the logs of your build server seems like the crash happened with a 
`_gmodules` test.
Unfortunately, the gmodules tests are still skipped on Linux:

  Collected 8 tests
  
  1: test_ref_and_ptr_dsym 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... 
skipped 'test case does not fall in any category of interest for this run'
  2: test_ref_and_ptr_dwarf 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... ok
  3: test_ref_and_ptr_dwo 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... ok
  4: test_ref_and_ptr_gmodules 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... 
skipped 'test case does not fall in any category of interest for this run'
  5: test_with_run_command_dsym 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... 
skipped 'test case does not fall in any category of interest for this run'
  6: test_with_run_command_dwarf 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... ok
  7: test_with_run_command_dwo 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... ok
  8: test_with_run_command_gmodules 
(TestDataFormatterLibcxxVector.LibcxxVectorDataFormatterTestCase)
 Test that that file and class static variables display correctly. ... 
skipped 'test case does not fall in any category of interest for this run'
  
  --
  Ran 8 tests in 1.260s

This  is aligned with the content of `test_categories.py`:

  def is_supported_on_platform(category, platform, compiler_path):
  if category == "dwo":
  # -gsplit-dwarf is not implemented by clang on Windows.
  return platform in ["linux", "freebsd"]
  elif category == "dsym":
  return platform in ["darwin", "macosx", "ios", "watchos", "tvos", 
"bridgeos"]
  elif category == "gmodules":
  # First, check to see if the platform can even support gmodules.
  if platform not in ["freebsd", "darwin", "macosx", "ios", "watchos", 
"tvos", "bridgeos"]:
  return False
  return gmodules.is_compiler_clang_with_gmodules(compiler_path)
  return True

So apparently I cannot run `gmodules` tests on Linux.

My next step is trying to reproduce the issue on a real MacOS laptop.


Repository:
  rC Clang

https://reviews.llvm.org/D53697



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

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347306: [ASTImporter] Set redecl chain of functions before 
any other import (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53702?vs=171093=174763#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53702

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -3144,19 +3144,6 @@
 FromConstructor->isExplicit(),
 D->isInlineSpecified(), D->isImplicit(), D->isConstexpr()))
   return ToFunction;
-if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
-  SmallVector CtorInitializers(NumInitializers);
-  // Import first, then allocate memory and copy if there was no error.
-  if (Error Err = ImportContainerChecked(
-  FromConstructor->inits(), CtorInitializers))
-return std::move(Err);
-  auto **Memory =
-  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
-  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
-  auto *ToCtor = cast(ToFunction);
-  ToCtor->setCtorInitializers(Memory);
-  ToCtor->setNumCtorInitializers(NumInitializers);
-}
   } else if (isa(D)) {
 if (GetImportedOrCreateDecl(
 ToFunction, D, Importer.getToContext(), cast(DC),
@@ -3184,6 +3171,30 @@
   return ToFunction;
   }
 
+  // Connect the redecl chain.
+  if (FoundByLookup) {
+auto *Recent = const_cast(
+  FoundByLookup->getMostRecentDecl());
+ToFunction->setPreviousDecl(Recent);
+  }
+
+  // Import Ctor initializers.
+  if (auto *FromConstructor = dyn_cast(D)) {
+if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
+  SmallVector CtorInitializers(NumInitializers);
+  // Import first, then allocate memory and copy if there was no error.
+  if (Error Err = ImportContainerChecked(
+  FromConstructor->inits(), CtorInitializers))
+return std::move(Err);
+  auto **Memory =
+  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
+  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
+  auto *ToCtor = cast(ToFunction);
+  ToCtor->setCtorInitializers(Memory);
+  ToCtor->setNumCtorInitializers(NumInitializers);
+}
+  }
+
   ToFunction->setQualifierInfo(ToQualifierLoc);
   ToFunction->setAccess(D->getAccess());
   ToFunction->setLexicalDeclContext(LexicalDC);
@@ -3199,12 +3210,6 @@
   }
   ToFunction->setParams(Parameters);
 
-  if (FoundByLookup) {
-auto *Recent = const_cast(
-  FoundByLookup->getMostRecentDecl());
-ToFunction->setPreviousDecl(Recent);
-  }
-
   // We need to complete creation of FunctionProtoTypeLoc manually with setting
   // params it refers to.
   if (TInfo) {


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -3144,19 +3144,6 @@
 FromConstructor->isExplicit(),
 D->isInlineSpecified(), D->isImplicit(), D->isConstexpr()))
   return ToFunction;
-if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
-  SmallVector CtorInitializers(NumInitializers);
-  // Import first, then allocate memory and copy if there was no error.
-  if (Error Err = ImportContainerChecked(
-  FromConstructor->inits(), CtorInitializers))
-return std::move(Err);
-  auto **Memory =
-  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
-  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
-  auto *ToCtor = cast(ToFunction);
-  ToCtor->setCtorInitializers(Memory);
-  ToCtor->setNumCtorInitializers(NumInitializers);
-}
   } else if (isa(D)) {
 if (GetImportedOrCreateDecl(
 ToFunction, D, Importer.getToContext(), cast(DC),
@@ -3184,6 +3171,30 @@
   return ToFunction;
   }
 
+  // Connect the redecl chain.
+  if (FoundByLookup) {
+auto *Recent = const_cast(
+  FoundByLookup->getMostRecentDecl());
+ToFunction->setPreviousDecl(Recent);
+  }
+
+  // Import Ctor initializers.
+  if (auto *FromConstructor = dyn_cast(D)) {
+if (unsigned NumInitializers = FromConstructor->getNumCtorInitializers()) {
+  SmallVector CtorInitializers(NumInitializers);
+  // Import first, then allocate memory and copy if there was no error.
+  if (Error Err = ImportContainerChecked(
+  FromConstructor->inits(), CtorInitializers))
+return std::move(Err);
+  auto **Memory =
+  new (Importer.getToContext()) CXXCtorInitializer *[NumInitializers];
+  std::copy(CtorInitializers.begin(), CtorInitializers.end(), Memory);
+  auto *ToCtor = cast(ToFunction);
+  

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

I have reproduced the issue on MacOS, fixed it and updated the patch 
accordingly.
Soon I will commit and will monitor if any build bots are being broken.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53697



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

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347648: [ASTImporter] Typedef import brings in the complete 
type (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53693?vs=171478=175422#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53693

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2311,9 +2311,16 @@
   if (!FoundDecl->isInIdentifierNamespace(IDNS))
 continue;
   if (auto *FoundTypedef = dyn_cast(FoundDecl)) {
-if (Importer.IsStructurallyEquivalent(
-D->getUnderlyingType(), FoundTypedef->getUnderlyingType()))
+QualType FromUT = D->getUnderlyingType();
+QualType FoundUT = FoundTypedef->getUnderlyingType();
+if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
+  // If the "From" context has a complete underlying type but we
+  // already have a complete underlying type then return with that.
+  if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
 return Importer.MapImported(D, FoundTypedef);
+}
+// FIXME Handle redecl chain.
+break;
   }
 
   ConflictingDecls.push_back(FoundDecl);
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3764,6 +3764,38 @@
   }
 }
 
+TEST_P(ASTImporterTestBase, ImportingTypedefShouldImportTheCompleteType) {
+  // We already have an incomplete underlying type in the "To" context.
+  auto Code =
+  R"(
+  template 
+  struct S {
+void foo();
+  };
+  using U = S;
+  )";
+  Decl *ToTU = getToTuDecl(Code, Lang_CXX11);
+  auto *ToD = FirstDeclMatcher().match(ToTU,
+  typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+
+  // The "From" context has the same typedef, but the underlying type is
+  // complete this time.
+  Decl *FromTU = getTuDecl(std::string(Code) +
+  R"(
+  void foo(U* u) {
+u->foo();
+  }
+  )", Lang_CXX11);
+  auto *FromD = FirstDeclMatcher().match(FromTU,
+  typedefNameDecl(hasName("U")));
+  ASSERT_FALSE(FromD->getUnderlyingType()->isIncompleteType());
+
+  // The imported type should be complete.
+  auto *ImportedD = cast(Import(FromD, Lang_CXX11));
+  EXPECT_FALSE(ImportedD->getUnderlyingType()->isIncompleteType());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
 ::testing::Values(ArgVector()), );
 


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2311,9 +2311,16 @@
   if (!FoundDecl->isInIdentifierNamespace(IDNS))
 continue;
   if (auto *FoundTypedef = dyn_cast(FoundDecl)) {
-if (Importer.IsStructurallyEquivalent(
-D->getUnderlyingType(), FoundTypedef->getUnderlyingType()))
+QualType FromUT = D->getUnderlyingType();
+QualType FoundUT = FoundTypedef->getUnderlyingType();
+if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
+  // If the "From" context has a complete underlying type but we
+  // already have a complete underlying type then return with that.
+  if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
 return Importer.MapImported(D, FoundTypedef);
+}
+// FIXME Handle redecl chain.
+break;
   }
 
   ConflictingDecls.push_back(FoundDecl);
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3764,6 +3764,38 @@
   }
 }
 
+TEST_P(ASTImporterTestBase, ImportingTypedefShouldImportTheCompleteType) {
+  // We already have an incomplete underlying type in the "To" context.
+  auto Code =
+  R"(
+  template 
+  struct S {
+void foo();
+  };
+  using U = S;
+  )";
+  Decl *ToTU = getToTuDecl(Code, Lang_CXX11);
+  auto *ToD = FirstDeclMatcher().match(ToTU,
+  typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+
+  // The "From" context has the same typedef, but the underlying type is
+  // complete this time.
+  Decl *FromTU = getTuDecl(std::string(Code) +
+  R"(
+  void foo(U* u) {
+u->foo();
+  }
+  )", Lang_CXX11);
+  auto 

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347564: [ASTImporter][Structural Eq] Check for 
isBeingDefined (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53697

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1740,8 +1740,9 @@
 return Err;
 
   // Add base classes.
-  if (auto *ToCXX = dyn_cast(To)) {
-auto *FromCXX = cast(From);
+  auto *ToCXX = dyn_cast(To);
+  auto *FromCXX = dyn_cast(From);
+  if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
 
 struct CXXRecordDecl::DefinitionData  = ToCXX->data();
 struct CXXRecordDecl::DefinitionData  = FromCXX->data();
Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -1016,7 +1016,8 @@
 return false;
 
   // Compare the definitions of these two records. If either or both are
-  // incomplete, we assume that they are equivalent.
+  // incomplete (i.e. it is a forward decl), we assume that they are
+  // equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
@@ -1031,6 +1032,11 @@
 if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
   return true;
 
+  // If one definition is currently being defined, we do not compare for
+  // equality and we assume that the decls are equal.
+  if (D1->isBeingDefined() || D2->isBeingDefined())
+return true;
+
   if (auto *D1CXX = dyn_cast(D1)) {
 if (auto *D2CXX = dyn_cast(D2)) {
   if (D1CXX->hasExternalLexicalStorage() &&
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3725,6 +3725,45 @@
   EXPECT_EQ(To1->getPreviousDecl(), To0);
 }
 
+TEST_P(ASTImporterTestBase,
+ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+  }
+
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B {
+  void f();
+  B* b;
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+// We expect no (ODR) warning during the import.
+auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
 ::testing::Values(ArgVector()), );
 


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1740,8 +1740,9 @@
 return Err;
 
   // Add base classes.
-  if (auto *ToCXX = dyn_cast(To)) {
-auto *FromCXX = cast(From);
+  auto *ToCXX = dyn_cast(To);
+  auto *FromCXX = dyn_cast(From);
+  if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
 
 struct CXXRecordDecl::DefinitionData  = ToCXX->data();
 struct CXXRecordDecl::DefinitionData  = FromCXX->data();
Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -1016,7 +1016,8 @@
 return false;
 
   // Compare the definitions of these two records. If either or both are
-  // incomplete, we assume that they are equivalent.
+  // incomplete (i.e. it is a forward decl), we assume that they are
+  // equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
@@ -1031,6 +1032,11 @@
 if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
   return true;
 
+  // If one definition is currently being defined, we do not compare for
+  // equality and we assume that 

[PATCH] D54898: Set MustBuildLookupTable on PrimaryContext in ExternalASTMerger

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM. Thank you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D54898



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

[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: cfe/trunk/unittests/AST/ASTImporterTest.cpp:3780
+  typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+

shafik wrote:
> As far as I can tell `S` is complete in `Code`, am I missing something? 
No, `S` is incomplete at this point. The specialization is indeed created, 
but its type is incomplete. There is no need for the type to be complete 
because there is not any operation which would require the size of the type.
This is how the AST looks like:
```
|-ClassTemplateDecl 0x1f5b378  line:3:14 S
| |-TemplateTypeParmDecl 0x1f5b228  col:26 typename depth 0 
index 0 T
| |-CXXRecordDecl 0x1f5b2e0  line:3:14 struct S definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod 
trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x1f5b5e0  col:14 implicit struct S
| | `-CXXMethodDecl 0x1f5b700  col:14 foo 'void ()'
| `-ClassTemplateSpecializationDecl 0x1f5b7d8  line:3:14 
struct S
|   `-TemplateArgument type 'int'
`-TypeAliasDecl 0x1f5b988  col:13 U 'S':'S'
  `-TemplateSpecializationType 0x1f5b8e0 'S' sugar S
|-TemplateArgument type 'int'
`-RecordType 0x1f5b8c0 'S'
  `-ClassTemplateSpecialization 0x1f5b7d8 'S'
```

However, below by using the member access operator (`->`) we require the 
completion of the type. Alas, the specialization has a definition there, not 
just a fwd declaration.
The AST of `FromTU` looks like this:
```
|-ClassTemplateDecl 0x2033c28  line:3:14 S
| |-TemplateTypeParmDecl 0x2033ad8  col:26 typename depth 0 
index 0 T
| |-CXXRecordDecl 0x2033b90  line:3:14 struct S definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod 
trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x2033e90  col:14 implicit struct S
| | `-CXXMethodDecl 0x2033fb0  col:14 foo 'void ()'
| `-ClassTemplateSpecializationDecl 0x2034088  line:3:14 
struct S definition
|   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
|   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
|   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
|   | |-MoveConstructor exists simple trivial needs_implicit
|   | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
|   | |-MoveAssignment exists simple trivial needs_implicit
|   | `-Destructor simple irrelevant trivial needs_implicit
|   |-TemplateArgument type 'int'
|   |-CXXRecordDecl 0x2034598 prev 0x2034088  col:14 implicit 
struct S
|   `-CXXMethodDecl 0x2034630  col:14 used foo 'void ()'
|-TypeAliasDecl 0x2034238  col:13 referenced U 
'S':'S'
| `-TemplateSpecializationType 0x2034190 'S' sugar S
|   |-TemplateArgument type 'int'
|   `-RecordType 0x2034170 'S'
| `-ClassTemplateSpecialization 0x2034088 'S'
`-FunctionDecl 0x2034420  line:8:12 foo 'void (U *)'
  |-ParmVarDecl 0x2034318  col:19 used u 'U *'
  `-CompoundStmt 0x2061a38 
`-CXXMemberCallExpr 0x2061a10  'void'
  `-MemberExpr 0x2034718  '' 
->foo 0x2034630
`-ImplicitCastExpr 0x2034700  'U *' 
  `-DeclRefExpr 0x2034508  'U *' lvalue ParmVar 0x2034318 'u' 'U 
*'
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53693



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

[PATCH] D53818: [ASTImporter] Changed use of Import to Import_New in ASTImporter.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> I reverted this change because it breaks a bunch of lldb tests (on MacOS, and 
> probably on other platforms too). To be clear, part of the reason I'm 
> reacting strongly here is that this is not the first patch this has come up 
> on. I'm worried about the broader trend of landing patches to ASTImporter 
> without proper reviews and testing as I am of this very instance. I really 
> think you should consider asking reviews to somebody who's familiar with 
> clang and test lldb as it's the main client of this functionality we have in 
> tree.

@davide I am terribly sorry about this issue. I just applied this patch on my 
Linux box, and it did not failed those tests which failed on the green lab 
buildbots. Could you please refer to the Linux buildbots which failed? Seems 
like there is a significant difference between the testsuites on Linux and 
MacOS. Our primary target is Linux and we do run the LLDB tests before we 
accept any commit into our fork and only then we continue with upstreaming to 
Phabricator.

Our platform in our CI is Linux only, but we are in the middle of getting a 
hosted MacOS to support our CI. I do hope that in the near future there will be 
no such issues. In the meantime, is there a way for us to execute a green lab 
MacOS build bot with a specific patch before a commit? If that is not possible, 
then we can just promise that we will monitor your buildbots and we will do the 
revert.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53818



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

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Did you ever resolve the issue of libcxx tests not running 
> https://reviews.llvm.org/D53697

Oh, by the way this change is completely independent from that other patch.


Repository:
  rC Clang

https://reviews.llvm.org/D53702



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

[PATCH] D53702: [ASTImporter] Set redecl chain of functions before any other import

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> Did you ever resolve the issue of libcxx tests not running 
> https://reviews.llvm.org/D53697

Hi @shafik ,
Sorry for the late reaction, I was on a two weeks long vacation recently.
My priority is to make those libcxx tests pass, I did not forget. :)


Repository:
  rC Clang

https://reviews.llvm.org/D53702



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

[PATCH] D53751: [ASTImporter] Added Import functions for transition to new API.

[Gabor Marton via Phabricator via cfe-commits]

martong added a subscriber: a_sidorin.
martong added a comment.

> I think these changes make sense at a high level but I am not sure about the 
> refactoring strategy. I am especially concerned we may end up in place where 
> all the effected users of the API don't get updated and we are stuck with 
> this parallel API.
>  Tagging in @rsmith since he has reviewed a lot of recent changes involving 
> ASTImpoter that I have seen recently and he will have a better feeling for 
> how these types of refactoring on handled on the clang side. I am mostly 
> focused on the lldb side but care about the ASTImporter since we depend on it.

Hi @shafik ,

I completely understand your concern. We modify ASTImporter in order to make 
cross translation unit (CTU) analysis working on C++ projects. During these 
modifications we carefully try to keep LLDB functionality intact.

This patch is one of the last patches of a refactor in ASTImporter about using 
`Error` and `Expected` as return types. We need this in CTU analysis because

- we want to distinguish between different kind of errors
- internally in ASTImporter we'd like to enforce the checking whether there has 
been any error during the import of any subexpressions

After this patch our next patch would rename these `Import_New` functions to 
`Import` and the old `Import` function implementations returning with a raw 
pointer would be deleted. This indeed would effect LLDB, thus we are going to 
create an LLDB patch too. We already have that LLDB change in our fork 
(https://github.com/Ericsson/lldb/commit/7085d20) and soon we will put that in 
Phabricator too.

Now, my concern is that waiting for the approve from @rsmith could take several 
months since he is usually very overwhelmed. Unfortunately, we have several 
other changes which depend on this change, so this is a blocker for us. Also, I 
think that @a_sidorin has the greatest experience in ASTImporter code. Would it 
be okay for you to accept this patch once Alexei approved it?


Repository:
  rC Clang

https://reviews.llvm.org/D53751



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

[PATCH] D53697: [ASTImporter][Structural Eq] Check for isBeingDefined

[Gabor Marton via Phabricator via cfe-commits]

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

- Get data of CXXRecordDecl only if set


Repository:
  rC Clang

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

https://reviews.llvm.org/D53697

Files:
  lib/AST/ASTImporter.cpp
  lib/AST/ASTStructuralEquivalence.cpp
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3725,6 +3725,45 @@
   EXPECT_EQ(To1->getPreviousDecl(), To0);
 }
 
+TEST_P(ASTImporterTestBase,
+ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+  }
+
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B {
+  void f();
+  B* b;
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+// We expect no (ODR) warning during the import.
+auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
 ::testing::Values(ArgVector()), );
 
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -1016,7 +1016,8 @@
 return false;
 
   // Compare the definitions of these two records. If either or both are
-  // incomplete, we assume that they are equivalent.
+  // incomplete (i.e. it is a forward decl), we assume that they are
+  // equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
@@ -1031,6 +1032,11 @@
 if (D1->hasExternalLexicalStorage() || D2->hasExternalLexicalStorage())
   return true;
 
+  // If one definition is currently being defined, we do not compare for
+  // equality and we assume that the decls are equal.
+  if (D1->isBeingDefined() || D2->isBeingDefined())
+return true;
+
   if (auto *D1CXX = dyn_cast(D1)) {
 if (auto *D2CXX = dyn_cast(D2)) {
   if (D1CXX->hasExternalLexicalStorage() &&
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1740,8 +1740,9 @@
 return Err;
 
   // Add base classes.
-  if (auto *ToCXX = dyn_cast(To)) {
-auto *FromCXX = cast(From);
+  auto *ToCXX = dyn_cast(To);
+  auto *FromCXX = dyn_cast(From);
+  if (ToCXX && FromCXX && ToCXX->dataPtr() && FromCXX->dataPtr()) {
 
 struct CXXRecordDecl::DefinitionData  = ToCXX->data();
 struct CXXRecordDecl::DefinitionData  = FromCXX->data();


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3725,6 +3725,45 @@
   EXPECT_EQ(To1->getPreviousDecl(), To0);
 }
 
+TEST_P(ASTImporterTestBase,
+ImportShouldNotReportFalseODRErrorWhenRecordIsBeingDefined) {
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+  }
+
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B {
+  void f();
+  B* b;
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+
+// We expect no (ODR) warning during the import.
+auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
 ::testing::Values(ArgVector()), );
 
Index: lib/AST/ASTStructuralEquivalence.cpp

[PATCH] D55280: [CTU] Make loadExternalAST return with non nullptr on success

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC350521: [CTU] Make loadExternalAST return with non nullptr 
on success (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55280?vs=176779=180479#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55280

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp


Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -208,9 +208,6 @@
   if (!ASTUnitOrError)
 return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
-  if (!Unit)
-return llvm::make_error(
-index_error_code::failed_to_get_external_ast);
   assert(>getFileManager() ==
  >getASTContext().getSourceManager().getFileManager());
 
@@ -324,6 +321,9 @@
   } else {
 Unit = FnUnitCacheEntry->second;
   }
+  if (!Unit)
+return llvm::make_error(
+index_error_code::failed_to_get_external_ast);
   return Unit;
 }
 
Index: include/clang/CrossTU/CrossTranslationUnit.h
===
--- include/clang/CrossTU/CrossTranslationUnit.h
+++ include/clang/CrossTU/CrossTranslationUnit.h
@@ -130,8 +130,9 @@
   /// \p IndexName. In case the declaration is found in the index the
   /// corresponding AST file will be loaded.
   ///
-  /// \return Returns an ASTUnit that contains the definition of the looked up
-  /// function.
+  /// \return Returns a pointer to the ASTUnit that contains the definition of
+  /// the looked up function or an Error.
+  /// The returned pointer is never a nullptr.
   ///
   /// Note that the AST files should also be in the \p CrossTUDir.
   llvm::Expected loadExternalAST(StringRef LookupName,


Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -208,9 +208,6 @@
   if (!ASTUnitOrError)
 return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
-  if (!Unit)
-return llvm::make_error(
-index_error_code::failed_to_get_external_ast);
   assert(>getFileManager() ==
  >getASTContext().getSourceManager().getFileManager());
 
@@ -324,6 +321,9 @@
   } else {
 Unit = FnUnitCacheEntry->second;
   }
+  if (!Unit)
+return llvm::make_error(
+index_error_code::failed_to_get_external_ast);
   return Unit;
 }
 
Index: include/clang/CrossTU/CrossTranslationUnit.h
===
--- include/clang/CrossTU/CrossTranslationUnit.h
+++ include/clang/CrossTU/CrossTranslationUnit.h
@@ -130,8 +130,9 @@
   /// \p IndexName. In case the declaration is found in the index the
   /// corresponding AST file will be loaded.
   ///
-  /// \return Returns an ASTUnit that contains the definition of the looked up
-  /// function.
+  /// \return Returns a pointer to the ASTUnit that contains the definition of
+  /// the looked up function or an Error.
+  /// The returned pointer is never a nullptr.
   ///
   /// Note that the AST files should also be in the \p CrossTUDir.
   llvm::Expected loadExternalAST(StringRef LookupName,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56441: [analyzer][CrossTU][NFC] Generalize to external definitions instead of external functions

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

Looks good to me (but xazax's comments are valid)! Thank you.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56441



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

[PATCH] D56581: [ASTImporter] Set the described template if not set

[Gabor Marton via Phabricator via cfe-commits]

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

During the addition of an injected class type to a record it may happen
that a CXXRecordDecl in the redecl chain does not have a described
template set and this caused an assert in LLDB.


Repository:
  rC Clang

https://reviews.llvm.org/D56581

Files:
  lib/AST/ASTImporter.cpp


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2734,6 +2734,9 @@
 getCanonicalForwardRedeclChain(D2CXX);
 for (auto *R : Redecls) {
   auto *RI = cast(R);
+  // Existing Decl in the chain might not have the described
+  // template set, so we set it now.
+  RI->setDescribedClassTemplate(ToDescribed);
   RI->setTypeForDecl(nullptr);
   // Below we create a new injected type and assign that to the
   // canonical decl, subsequent declarations in the chain will reuse


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2734,6 +2734,9 @@
 getCanonicalForwardRedeclChain(D2CXX);
 for (auto *R : Redecls) {
   auto *RI = cast(R);
+  // Existing Decl in the chain might not have the described
+  // template set, so we set it now.
+  RI->setDescribedClassTemplate(ToDescribed);
   RI->setTypeForDecl(nullptr);
   // Below we create a new injected type and assign that to the
   // canonical decl, subsequent declarations in the chain will reuse
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55646: [ASTImporter] Make ODR diagnostics warning by default

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Just a quick note. We are pretty sure that `StructuralEquivalency` can have 
false positive results, i.e. it can report two decls as nonequivalent falsely. 
My understanding is that we should report an error only if we are absolutely 
certain that an error has happened, this is not the case with 
`StructuralEquivalency`. Consequently this should be a warning in Sema 
(modules) too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55646



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

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:1441
+  To->setInit(ToInit);
+  if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();

a_sidorin wrote:
> I see that this is only a code move but I realized that ASTReader and 
> ASTImporter handle this case differently. ASTReader code says:
> 
> ```
> if (Val > 1) { // IsInitKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
>   EvaluatedStmt *Eval = VD->ensureEvaluatedStmt();
>   Eval->CheckedICE = true;
>   Eval->IsICE = Val == 3;
> }
> ```
> but ASTimporter sets these bits only if `isInitKnownICE()` is `true`. This 
> looks strange.
The comment in ASTReader seems to be wrong and misleading.
I checked the correspondent code in ASTWriter:
```
Record.push_back(!D->isInitKnownICE() ? 1 : (D->isInitICE() ? 3 : 2));
```
Thus, the comment in ASTReader should be:
```
if (Val > 1) { // IsInitNOTKnownICE = 1, IsInitNotICE = 2, IsInitICE = 3
```
So, `Val > 1` means that the original init expression written by the ASTWriter 
had the ICE-ness already determined.
Thus the ASTImporter code seems correct to me. 


Repository:
  rC Clang

https://reviews.llvm.org/D51597



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

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

[Gabor Marton via Phabricator via cfe-commits]

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

- Fix formatting and typo


Repository:
  rC Clang

https://reviews.llvm.org/D51597

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1828,13 +1828,62 @@
   {
 Decl *FromTU =
 getTuDecl("extern int x; int f() { return x; }", Lang_CXX, "input2.cc");
-auto *FromD =
-FirstDeclMatcher().match(FromTU, functionDecl());
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
 Import(FromD, Lang_CXX);
   }
   EXPECT_TRUE(Imported2->isUsed(false));
 }
 
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag2) {
+  auto Pattern = varDecl(hasName("x"));
+  VarDecl *ExistingD;
+  {
+Decl *ToTU = getToTuDecl("int x = 1;", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+  }
+  EXPECT_FALSE(ExistingD->isUsed(false));
+  {
+Decl *FromTU = getTuDecl(
+"int x = 1; int f() { return x; }", Lang_CXX, "input1.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+  }
+  EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
+TEST_P(ASTImporterTestBase, ImportDoesUpdateUsedFlag3) {
+  auto Pattern = varDecl(hasName("a"));
+  VarDecl *ExistingD;
+  {
+Decl *ToTU = getToTuDecl(
+R"(
+struct A {
+  static const int a = 1;
+};
+)", Lang_CXX);
+ExistingD = FirstDeclMatcher().match(ToTU, Pattern);
+  }
+  EXPECT_FALSE(ExistingD->isUsed(false));
+  {
+Decl *FromTU = getTuDecl(
+R"(
+struct A {
+  static const int a = 1;
+};
+const int *f() { return ::a; } // requires storage,
+ // thus used flag will be set
+)", Lang_CXX, "input1.cc");
+auto *FromFunD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+auto *FromD = FirstDeclMatcher().match(FromTU, Pattern);
+ASSERT_TRUE(FromD->isUsed(false));
+Import(FromFunD, Lang_CXX);
+  }
+  EXPECT_TRUE(ExistingD->isUsed(false));
+}
+
 TEST_P(ASTImporterTestBase, ReimportWithUsedFlag) {
   auto Pattern = varDecl(hasName("x"));
 
@@ -3244,6 +3293,94 @@
   EXPECT_TRUE(ToInitExpr->isGLValue());
 }
 
+struct ImportVariables : ASTImporterTestBase {};
+
+TEST_P(ImportVariables, ImportOfOneDeclBringsInTheWholeChain) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {
+static const int a = 1 + 2;
+  };
+  const int A::a;
+  )", Lang_CXX, "input1.cc");
+
+  auto *FromDWithInit = FirstDeclMatcher().match(
+  FromTU, varDecl(hasName("a"))); // Decl with init
+  auto *FromDWithDef = LastDeclMatcher().match(
+  FromTU, varDecl(hasName("a"))); // Decl with definition
+  ASSERT_NE(FromDWithInit, FromDWithDef);
+  ASSERT_EQ(FromDWithDef->getPreviousDecl(), FromDWithInit);
+
+  auto *ToD0 = cast(Import(FromDWithInit, Lang_CXX11));
+  auto *ToD1 = cast(Import(FromDWithDef, Lang_CXX11));
+  ASSERT_TRUE(ToD0);
+  ASSERT_TRUE(ToD1);
+  EXPECT_NE(ToD0, ToD1);
+  EXPECT_EQ(ToD1->getPreviousDecl(), ToD0);
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInDifferentTUs) {
+  auto StructA =
+  R"(
+  struct A {
+static const int a = 1 + 2;
+  };
+  )";
+  Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+  Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a;", Lang_CXX,
+   "input1.cc");
+
+  auto *FromDWithInit = FirstDeclMatcher().match(
+  FromTU, varDecl(hasName("a"))); // Decl with init
+  auto *FromDWithDef = LastDeclMatcher().match(
+  FromTU, varDecl(hasName("a"))); // Decl with definition
+  ASSERT_EQ(FromDWithInit, FromDWithDef->getPreviousDecl());
+  ASSERT_TRUE(FromDWithInit->getInit());
+  ASSERT_FALSE(FromDWithInit->isThisDeclarationADefinition());
+  ASSERT_TRUE(FromDWithDef->isThisDeclarationADefinition());
+  ASSERT_FALSE(FromDWithDef->getInit());
+
+  auto *ToD = FirstDeclMatcher().match(
+  ToTU, varDecl(hasName("a"))); // Decl with init
+  ASSERT_TRUE(ToD->getInit());
+  ASSERT_FALSE(ToD->getDefinition());
+
+  auto *ImportedD = cast(Import(FromDWithDef, Lang_CXX11));
+  EXPECT_TRUE(ImportedD->getAnyInitializer());
+  EXPECT_TRUE(ImportedD->getDefinition());
+}
+
+TEST_P(ImportVariables, InitAndDefinitionAreInTheFromContext) {
+  auto StructA =
+  R"(
+  struct A {
+static const int a;
+  };
+  )";
+  Decl *ToTU = getToTuDecl(StructA, Lang_CXX);
+  Decl *FromTU = getTuDecl(std::string(StructA) + "const int A::a = 1 + 2;",
+   Lang_CXX, "input1.cc");
+
+  auto *FromDDeclarationOnly = FirstDeclMatcher().match(
+  FromTU, varDecl(hasName("a")));
+  auto *FromDWithDef = LastDeclMatcher().match(
+ 

[PATCH] D51597: [ASTImporter] Fix import of VarDecl init

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342384: [ASTImporter] Fix import of VarDecl init (authored 
by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51597

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -107,9 +107,11 @@
   }
 
   SmallVector getCanonicalForwardRedeclChain(Decl* D) {
-// Currently only FunctionDecl is supported
-auto FD = cast(D);
-return getCanonicalForwardRedeclChain(FD);
+if (auto *FD = dyn_cast(D))
+  return getCanonicalForwardRedeclChain(FD);
+if (auto *VD = dyn_cast(D))
+  return getCanonicalForwardRedeclChain(VD);
+llvm_unreachable("Bad declaration kind");
   }
 
   void updateFlags(const Decl *From, Decl *To) {
@@ -280,10 +282,9 @@
  (IDK == IDK_Default && !Importer.isMinimalImport());
 }
 
+bool ImportInitializer(VarDecl *From, VarDecl *To);
 bool ImportDefinition(RecordDecl *From, RecordDecl *To,
   ImportDefinitionKind Kind = IDK_Default);
-bool ImportDefinition(VarDecl *From, VarDecl *To,
-  ImportDefinitionKind Kind = IDK_Default);
 bool ImportDefinition(EnumDecl *From, EnumDecl *To,
   ImportDefinitionKind Kind = IDK_Default);
 bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To,
@@ -1424,18 +1425,26 @@
   return false;
 }
 
-bool ASTNodeImporter::ImportDefinition(VarDecl *From, VarDecl *To,
-   ImportDefinitionKind Kind) {
+bool ASTNodeImporter::ImportInitializer(VarDecl *From, VarDecl *To) {
   if (To->getAnyInitializer())
 return false;
 
-  // FIXME: Can we really import any initializer? Alternatively, we could force
-  // ourselves to import every declaration of a variable and then only use
-  // getInit() here.
-  To->setInit(Importer.Import(const_cast(From->getAnyInitializer(;
+  Expr *FromInit = From->getInit();
+  if (!FromInit)
+return false;
 
-  // FIXME: Other bits to merge?
+  Expr *ToInit = Importer.Import(const_cast(FromInit));
+  if (!ToInit)
+return true;
 
+  To->setInit(ToInit);
+  if (From->isInitKnownICE()) {
+EvaluatedStmt *Eval = To->ensureEvaluatedStmt();
+Eval->CheckedICE = true;
+Eval->IsICE = From->isInitICE();
+  }
+
+  // FIXME: Other bits to merge?
   return false;
 }
 
@@ -3138,6 +3147,16 @@
 }
 
 Decl *ASTNodeImporter::VisitVarDecl(VarDecl *D) {
+
+  SmallVector Redecls = getCanonicalForwardRedeclChain(D);
+  auto RedeclIt = Redecls.begin();
+  // Import the first part of the decl chain. I.e. import all previous
+  // declarations starting from the canonical decl.
+  for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+if (!Importer.Import(*RedeclIt))
+  return nullptr;
+  assert(*RedeclIt == D);
+
   // Import the major distinguishing characteristics of a variable.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
@@ -3150,8 +3169,8 @@
 
   // Try to find a variable in our own ("to") context with the same name and
   // in the same context as the variable we're importing.
+  VarDecl *FoundByLookup = nullptr;
   if (D->isFileVarDecl()) {
-VarDecl *MergeWithVar = nullptr;
 SmallVector ConflictingDecls;
 unsigned IDNS = Decl::IDNS_Ordinary;
 SmallVector FoundDecls;
@@ -3166,7 +3185,23 @@
 D->hasExternalFormalLinkage()) {
   if (Importer.IsStructurallyEquivalent(D->getType(),
 FoundVar->getType())) {
-MergeWithVar = FoundVar;
+
+// The VarDecl in the "From" context has a definition, but in the
+// "To" context we already have a definition.
+VarDecl *FoundDef = FoundVar->getDefinition();
+if (D->isThisDeclarationADefinition() && FoundDef)
+  // FIXME Check for ODR error if the two definitions have
+  // different initializers?
+  return Importer.MapImported(D, FoundDef);
+
+// The VarDecl in the "From" context has an initializer, but in the
+// "To" context we already have an initializer.
+const VarDecl *FoundDInit = nullptr;
+if (D->getInit() && FoundVar->getAnyInitializer(FoundDInit))
+  // FIXME Diagnose ODR error if the two initializers are different?
+  return Importer.MapImported(D, const_cast(FoundDInit));
+
+FoundByLookup = FoundVar;
 break;
   }
 
@@ -3183,11 +3218,11 @@
 return nullptr;
 
   FoundVar->setType(T);
-  MergeWithVar = FoundVar;
+  FoundByLookup = FoundVar;
   break;
 } else if 

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:3046
+if (!D->doesThisDeclarationHaveABody())
+  return cast(const_cast(FoundByLookup));
+else {

balazske wrote:
> The `cast` should not be needed here (and is not done at the other 
> return places). The `const_cast` can be omitted too (`FoundByLookup` is not 
> const now).
We must also register the decl into the map of imported decls as we do in all 
the other cases.
```
return Importer.MapImported(D, FoundByLookup);
```


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

https://reviews.llvm.org/D56936



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

[PATCH] D56651: [ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

The updated version looks good to me! Thank you!


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

https://reviews.llvm.org/D56651



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

[PATCH] D56936: Fix handling of overriden methods during ASTImport

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Shafik,
I have realized what's the problem with the `ctu-main` test. When we import the 
body and set the new body to the existing FunctionDecl then the parameters are 
still the old parameters so the new body does not refer to the formal 
parameters! This way the analyzer legally thinks that the parameter is not used 
inside the function because there is no DeclRef to that :(
This could be solved only if we merge *every* parts precisely, including the 
parameters, body, noexcept specifier, etc. But this would be a huge work.

Also, the test in `ctu-main` contains ODR violations. E.g, below `fcl` is first 
just a protoype, then it is defined in-class in the second TU.

  // ctu-main.cpp
  class mycls {
  public:
int fcl(int x);
//...
  
  // ctu-other.cpp
   class mycls {
   public:
int fcl(int x) {
  return x + 5;
}
//...

In the second TU it should be defined out-of-class.

So my suggestion is to

1. use out-of-class definition of functions in `ctu-other.cpp`. Since I tried 
it already i have the diff for the lit tests, attached.F7837319: 
lit_tests.patch 
2. skip the case when there is a definition in the 'from' context and let it do 
the redecl chain.

For 2) I was thinking about this:

  if (FoundByLookup) {
if (auto *MD = dyn_cast(FoundByLookup)) {
  if (D->getLexicalDeclContext() == D->getDeclContext()) {
if (!D->doesThisDeclarationHaveABody())
  return cast(const_cast(FoundByLookup));
else {
  // Let's continue and build up the redecl chain in this case.
  // FIXME Merge the functions into one decl.
}
  }
}
  }

Later, we must implement the case when we have to merge the definition into the 
prototype properly, but that should be definitely another patch. 
Actually, this case comes up mostly with class template specializations , 
because different specializations may have a function prototype in one TU, but 
a definition for that in another TU (see e.g. the test 
`MergeFieldDeclsOfClassTemplateSpecialization`).




Comment at: lib/AST/ASTImporter.cpp:3042
 
+  if (FoundByLookup) {
+if (auto *MD = dyn_cast(FoundByLookup)) {

It is not trivial why we add this block to the code, so I think a comment would 
be really appreciated here.
I was thinking about something like this:
```
+  // We do not allow more than one in-class prototype of a function.  This is
+  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
+  // assumes that only one function can override a function. Building a redecl
+  // chain would result there are more than one function which override the
+  // base function (even if they are part of the same redecl chain inside the
+  // derived class.)
```


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

https://reviews.llvm.org/D56936



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

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Gabor Marton via Phabricator via cfe-commits]

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

I have created a separate patch for the test related refactor, this patch now
depends on that patch and contains merely the visibility related change and
their tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2264,6 +2264,266 @@
 }).match(ToTU, functionDecl()));
 }
 
+//FIXME Move these tests to a separate test file.
+namespace TypeAndValueParameterizedTests {
+
+// Type parameters for type-parameterized test fixtures.
+struct GetFunPattern {
+  using DeclTy = FunctionDecl;
+  BindableMatcher operator()() { return functionDecl(hasName("f")); }
+};
+struct GetVarPattern {
+  using DeclTy = VarDecl;
+  BindableMatcher operator()() { return varDecl(hasName("v")); }
+};
+
+// Values for the value-parameterized test fixtures.
+// FunctionDecl:
+auto *ExternF = "void f();";
+auto *StaticF = "static void f();";
+auto *AnonF = "namespace { void f(); }";
+// VarDecl:
+auto *ExternV = "extern int v;";
+auto *StaticV = "static int v;";
+auto *AnonV = "namespace { extern int v; }";
+
+// First value in tuple: Compile options.
+// Second value in tuple: Source code to be used in the test.
+using ImportVisibilityChainParams =
+::testing::WithParamInterface>;
+// Fixture to test the redecl chain of Decls with the same visibility.  Gtest
+// makes it possible to have either value-parameterized or type-parameterized
+// fixtures.  However, we cannot have both value- and type-parameterized test
+// fixtures.  This is a value-parameterized test fixture in the gtest sense. We
+// intend to mimic gtest's type-parameters via the PatternFactory template
+// parameter.  We manually instantiate the different tests with the each types.
+template 
+class ImportVisibilityChain
+: public ASTImporterTestBase, public ImportVisibilityChainParams {
+protected:
+  using DeclTy = typename PatternFactory::DeclTy;
+  ArgVector getExtraArgs() const override { return std::get<0>(GetParam()); }
+  std::string getCode() const { return std::get<1>(GetParam()); }
+  BindableMatcher getPattern() const { return PatternFactory()(); }
+
+  // Type-parameterized test.
+  void TypedTest_ImportChain() {
+std::string Code = getCode() + getCode();
+auto Pattern = getPattern();
+
+TranslationUnitDecl *FromTu = getTuDecl(Code, Lang_CXX, "input0.cc");
+
+auto *FromF0 = FirstDeclMatcher().match(FromTu, Pattern);
+auto *FromF1 = LastDeclMatcher().match(FromTu, Pattern);
+
+auto *ToF0 = Import(FromF0, Lang_CXX);
+auto *ToF1 = Import(FromF1, Lang_CXX);
+
+EXPECT_TRUE(ToF0);
+ASSERT_TRUE(ToF1);
+EXPECT_NE(ToF0, ToF1);
+EXPECT_EQ(ToF1->getPreviousDecl(), ToF0);
+  }
+};
+
+// Manual instantiation of the fixture with each type.
+using ImportFunctionsVisibilityChain = ImportVisibilityChain;
+using ImportVariablesVisibilityChain = ImportVisibilityChain;
+// Value-parameterized test for the first type.
+TEST_P(ImportFunctionsVisibilityChain, ImportChain) {
+  TypedTest_ImportChain();
+}
+// Value-parameterized test for the second type.
+TEST_P(ImportVariablesVisibilityChain, ImportChain) {
+  TypedTest_ImportChain();
+}
+
+// Automatic instantiation of the value-parameterized tests.
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionsVisibilityChain,
+::testing::Combine(
+   DefaultTestValuesForRunOptions,
+   ::testing::Values(ExternF, StaticF, AnonF)), );
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, ImportVariablesVisibilityChain,
+::testing::Combine(
+DefaultTestValuesForRunOptions,
+// There is no point to instantiate with StaticV, because in C++ we can
+// forward declare a variable only with the 'extern' keyword.
+// Consequently, each fwd declared variable has external linkage.  This
+// is different in the C language where any declaration without an
+// initializer is a tentative definition, subsequent definitions may be
+// provided but they must have the same linkage.  See also the test
+// ImportVariableChainInC which test for this special C Lang case.
+::testing::Values(ExternV, AnonV)), );
+
+// First value in tuple: Compile options.
+// Second value in tuple: Tuple with informations for the test.
+// Code for first import (or initial code), code to import, whether the `f`
+// functions are expected to be linked in a declaration chain.
+// One value of this tuple is combined with every value of compile options.
+// The test can have a single tuple as parameter only.
+using ImportVisibilityParams 

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

[Gabor Marton via Phabricator via cfe-commits]

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

Currently `TestImportBase` is derived from `ParameterizedTestsFixture`
which explicitly states that the gtest parameter can be only an
`ArgVector`. This is a limitation when we want to create tests which may
have different parameters.
E.g. we would like to create tests where we can combine different test
parameters. So, for example we'd like gtest to be able to provide
parameters of `` instead of a simple
`ArgVector`.


Repository:
  rC Clang

https://reviews.llvm.org/D57322

Files:
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -10,6 +10,10 @@
 //
 //===--===//
 
+// Define this to have ::testing::Combine available.
+// FIXME: Better solution for this?
+#define GTEST_HAS_COMBINE 1
+
 #include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
@@ -58,23 +62,31 @@
 // Common base for the different families of ASTImporter tests that are
 // parameterized on the compiler options which may result a different AST. E.g.
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:
+  // Return the extra arguments appended to runtime options at compilation.
+  virtual ArgVector getExtraArgs() const { return ArgVector(); }
 
   // Returns the argument vector used for a specific language option, this set
   // can be tweaked by the test parameters.
   ArgVector getArgVectorForLanguage(Language Lang) const {
 ArgVector Args = getBasicRunOptionsForLanguage(Lang);
-ArgVector ExtraArgs = GetParam();
+ArgVector ExtraArgs = getExtraArgs();
 for (const auto  : ExtraArgs) {
   Args.push_back(Arg);
 }
 return Args;
   }
-
 };
 
+auto DefaultTestValuesForRunOptions = ::testing::Values(
+ArgVector(), ArgVector{"-fdelayed-template-parsing"},
+ArgVector{"-fms-compatibility"},
+ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
+
 // Base class for those tests which use the family of `testImport` functions.
-class TestImportBase : public ParameterizedTestsFixture {
+class TestImportBase : public CompilerOptionSpecificTest,
+   public ::testing::WithParamInterface {
 
   template 
   NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter ,
@@ -159,6 +171,9 @@
 VerificationMatcher);
   }
 
+protected:
+  ArgVector getExtraArgs() const override { return GetParam(); }
+
 public:
 
   /// Test how AST node named "declToImport" located in the translation unit
@@ -284,7 +299,7 @@
 // This class provides generic methods to write tests which can check internal
 // attributes of AST nodes like getPreviousDecl(), isVirtual(), etc. Also,
 // this fixture makes it possible to import from several "From" contexts.
-class ASTImporterTestBase : public ParameterizedTestsFixture {
+class ASTImporterTestBase : public CompilerOptionSpecificTest {
 
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
@@ -450,6 +465,10 @@
 return FromTU->import(*LookupTablePtr, ToAST.get(), From);
   }
 
+  template  DeclT *Import(DeclT *From, Language Lang) {
+return cast_or_null(Import(cast(From), Lang));
+  }
+
   QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang) {
 lazyInitToAST(ToLang, "", OutputFileName);
 TU *FromTU = findFromTU(TUDecl);
@@ -473,11 +492,18 @@
   }
 };
 
+class ASTImporterOptionSpecificTestBase
+: public ASTImporterTestBase,
+  public ::testing::WithParamInterface {
+protected:
+  ArgVector getExtraArgs() const override { return GetParam(); }
+};
+
 struct ImportExpr : TestImportBase {};
 struct ImportType : TestImportBase {};
 struct ImportDecl : TestImportBase {};
 
-struct CanonicalRedeclChain : ASTImporterTestBase {};
+struct CanonicalRedeclChain : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(CanonicalRedeclChain, ShouldBeConsequentWithMatchers) {
   Decl *FromTU = getTuDecl("void f();", Lang_CXX);
@@ -1000,7 +1026,7 @@
  has(declStmt(hasSingleDecl(varDecl(hasName("d");
 }
 
-TEST_P(ASTImporterTestBase, ImportRecordTypeInFunc) {
+TEST_P(ASTImporterOptionSpecificTestBase, ImportRecordTypeInFunc) {
   Decl *FromTU = getTuDecl("int declToImport() { "
"  struct data_t {int a;int b;};"
"  struct data_t d;"
@@ -1015,7 +1041,7 @@
   EXPECT_FALSE(ToType.isNull());
 }
 
-TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncParams) {
+TEST_P(ASTImporterOptionSpecificTestBase, ImportRecordDeclInFuncParams) 

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Gabor Marton via Phabricator via cfe-commits]

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

- Remove dumpDeclContext() call
- Remove superfluous else


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -10,6 +10,10 @@
 //
 //===--===//
 
+// Define this to have ::testing::Combine available.
+// FIXME: Better solution for this?
+#define GTEST_HAS_COMBINE 1
+
 #include "clang/AST/ASTImporter.h"
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
@@ -58,23 +62,31 @@
 // Common base for the different families of ASTImporter tests that are
 // parameterized on the compiler options which may result a different AST. E.g.
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:
+  // Return the extra arguments appended to runtime options at compilation.
+  virtual ArgVector getExtraArgs() const { return ArgVector(); }
 
   // Returns the argument vector used for a specific language option, this set
   // can be tweaked by the test parameters.
   ArgVector getArgVectorForLanguage(Language Lang) const {
 ArgVector Args = getBasicRunOptionsForLanguage(Lang);
-ArgVector ExtraArgs = GetParam();
+ArgVector ExtraArgs = getExtraArgs();
 for (const auto  : ExtraArgs) {
   Args.push_back(Arg);
 }
 return Args;
   }
-
 };
 
+auto DefaultTestValuesForRunOptions = ::testing::Values(
+ArgVector(), ArgVector{"-fdelayed-template-parsing"},
+ArgVector{"-fms-compatibility"},
+ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
+
 // Base class for those tests which use the family of `testImport` functions.
-class TestImportBase : public ParameterizedTestsFixture {
+class TestImportBase : public CompilerOptionSpecificTest,
+   public ::testing::WithParamInterface {
 
   template 
   NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter ,
@@ -159,6 +171,9 @@
 VerificationMatcher);
   }
 
+protected:
+  ArgVector getExtraArgs() const override { return GetParam(); }
+
 public:
 
   /// Test how AST node named "declToImport" located in the translation unit
@@ -284,7 +299,7 @@
 // This class provides generic methods to write tests which can check internal
 // attributes of AST nodes like getPreviousDecl(), isVirtual(), etc. Also,
 // this fixture makes it possible to import from several "From" contexts.
-class ASTImporterTestBase : public ParameterizedTestsFixture {
+class ASTImporterTestBase : public CompilerOptionSpecificTest {
 
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
@@ -450,6 +465,10 @@
 return FromTU->import(*LookupTablePtr, ToAST.get(), From);
   }
 
+  template  DeclT *Import(DeclT *From, Language Lang) {
+return cast_or_null(Import(cast(From), Lang));
+  }
+
   QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang) {
 lazyInitToAST(ToLang, "", OutputFileName);
 TU *FromTU = findFromTU(TUDecl);
@@ -473,11 +492,18 @@
   }
 };
 
+class ASTImporterOptionSpecificTestBase
+: public ASTImporterTestBase,
+  public ::testing::WithParamInterface {
+protected:
+  ArgVector getExtraArgs() const override { return GetParam(); }
+};
+
 struct ImportExpr : TestImportBase {};
 struct ImportType : TestImportBase {};
 struct ImportDecl : TestImportBase {};
 
-struct CanonicalRedeclChain : ASTImporterTestBase {};
+struct CanonicalRedeclChain : ASTImporterOptionSpecificTestBase {};
 
 TEST_P(CanonicalRedeclChain, ShouldBeConsequentWithMatchers) {
   Decl *FromTU = getTuDecl("void f();", Lang_CXX);
@@ -1000,7 +1026,7 @@
  has(declStmt(hasSingleDecl(varDecl(hasName("d");
 }
 
-TEST_P(ASTImporterTestBase, ImportRecordTypeInFunc) {
+TEST_P(ASTImporterOptionSpecificTestBase, ImportRecordTypeInFunc) {
   Decl *FromTU = getTuDecl("int declToImport() { "
"  struct data_t {int a;int b;};"
"  struct data_t d;"
@@ -1015,7 +1041,7 @@
   EXPECT_FALSE(ToType.isNull());
 }
 
-TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncParams) {
+TEST_P(ASTImporterOptionSpecificTestBase, ImportRecordDeclInFuncParams) {
   // This construct is not supported by ASTImporter.
   Decl *FromTU = getTuDecl(
   "int declToImport(struct data_t{int a;int b;} ***d){ return 0; }",
@@ -1027,7 +1053,7 @@
   EXPECT_EQ(To, nullptr);
 }
 
-TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncFromMacro) {

[PATCH] D57232: [ASTImporter] Check visibility/linkage of functions and variables

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2954
+return Found->hasExternalFormalLinkage();
+  else if (Importer.GetFromTU(Found) == From->getTranslationUnitDecl()) {
+if (From->isInAnonymousNamespace())

shafik wrote:
> We don't really need an `else ` here and it would be more like an early exit 
> which is what llvm style guide suggests.
> 
> In the same vein I would do:
> 
> if (Importer.GetFromTU(Found) != From->getTranslationUnitDecl())
>return false; 
> 
> so a second early exit and remove a level of nesting at the same time.
It's a good catch, thanks.



Comment at: unittests/AST/ASTImporterTest.cpp:65
 // -fms-compatibility or -fdelayed-template-parsing.
-struct ParameterizedTestsFixture : ::testing::TestWithParam {
+class CompilerOptionSpecificTest : public ::testing::Test {
+protected:

shafik wrote:
> Are these changes directly related to the visibility change? There is a lot 
> of noise that is not obviously related to the description in the PR.
> 
> 
> If not maybe it should be a separate PR?
Actually, it will be more precise to create first a patch which contains the 
tests related refactor, I agree. I'll do that and this patch will be dependent 
upon that.



Comment at: unittests/AST/ASTImporterTest.cpp:2523
+  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  cast(ToTU)->dumpDeclContext();
+  ASSERT_EQ(DeclCounter().match(ToTU, 
functionDecl(hasName("f"))),

balazske wrote:
> Is this dump needed? (The test should not write unnecessary text output. But 
> debug statements can be leaved in the test, possibly in comment.)
Thanks for catching this, good point.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57232



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:2154
+return NameOrErr.takeError();
 }
   }

a_sidorin wrote:
> Should we write `else Name = *NameOrError`?
Atm, we implement the simplest strategy for name conflict handling: we just 
return with the error.
However, on a long term we should use the returned name indeed. I mean when we 
really do implement a renaming strategy via `HandleNameConflict`. 

Also, for that we'd have to double check that the `Name` is indeed used when we 
create the AST node. So I'd rather leave this `else` branch up to that point. 
Hopefully, by that time we'll have unittests which would exercise this `else` 
branch, now we just don't have any.



Repository:
  rC Clang

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

https://reviews.llvm.org/D59692



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

[PATCH] D55049: Changed every use of ASTImporter::Import to Import_New

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping @shafik @a_sidorin


Repository:
  rC Clang

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

https://reviews.llvm.org/D55049



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM!


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

https://reviews.llvm.org/D59665



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

[PATCH] D59845: Fix IsStructuralMatch specialization for EnumDecl to prevent re-importing an EnumDecl while trying to complete it

[Gabor Marton via Phabricator via cfe-commits]

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

LGTM!


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

https://reviews.llvm.org/D59845



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D59665#1442328 , @shafik wrote:

> @martong your idea does not work b/c default construction `DeclarationName()` 
> treats it the same as being empty. So `if (!Name)` is still `true`.


And did you try moving the `push_back` to the other scope? I'd expect the the 
ConflictingDecls to be empty then.


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

https://reviews.llvm.org/D59665



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Looks good for me now, but we should make a similar change in VisitRecordDecl 
too.


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

https://reviews.llvm.org/D59665



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2463
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, IDNS,
+  Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
  ConflictingDecls.data(),

shafik wrote:
> a_sidorin wrote:
> > If I understand correctly, this will replace Name with SearchName causing 
> > an anonymous enum to be imported as a named a few lines below. It doesn't 
> > look like a correct behaviour to me.
> This is correct because either `SearchName` is `Name` or it is the name of 
> the typedef for the anonymous enum set via `ImportInto(SearchName, 
> D->getTypedefNameForAnonDecl()->getDeclName())`
Okay, this is indeed correct. But then we should make a similar change in 
VisitRecordDecl too (there we do exactly the same with typedefs).


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

https://reviews.llvm.org/D59665



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

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

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

- Add braces around else
- Remove falsely duplicated push_back
- Use Expected in HandleNameConflict


Repository:
  rC Clang

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

https://reviews.llvm.org/D59692

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.c
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1964,7 +1964,7 @@
   auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
   // We expect one (ODR) warning during the import.
   EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
-  EXPECT_EQ(2u,
+  EXPECT_EQ(1u,
 DeclCounter().match(ToTU, recordDecl(hasName("X";
 }
 
@@ -2674,6 +2674,64 @@
 functionDecl(hasName("f"), hasDescendant(declRefExpr()));
 }
 
+struct ImportFunctionTemplates : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportFunctionTemplates,
+   ImportFunctionWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void foo(T) {}
+  void foo();
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl("void foo();", Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("foo")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportConstructorWhenThereIsAFunTemplateWithSameName) {
+  auto Code =
+  R"(
+  struct Foo {
+template 
+Foo(T) {}
+Foo();
+  };
+  )";
+  getToTuDecl(Code, Lang_CXX);
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto *FromD =
+  LastDeclMatcher().match(FromTU, cxxConstructorDecl());
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
+TEST_P(ImportFunctionTemplates,
+   ImportOperatorWhenThereIsAFunTemplateWithSameName) {
+  getToTuDecl(
+  R"(
+  template 
+  void operator<(T,T) {}
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X{};
+  void operator<(X, X);
+  )",
+  Lang_CXX);
+  auto *FromD = LastDeclMatcher().match(
+  FromTU, functionDecl(hasOverloadedOperatorName("<")));
+  auto *ImportedD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ImportedD);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
@@ -5534,6 +5592,9 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctionTemplates,
+DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFriendFunctionTemplates,
 DefaultTestValuesForRunOptions, );
 
Index: test/Analysis/Inputs/ctu-other.c
===
--- test/Analysis/Inputs/ctu-other.c
+++ test/Analysis/Inputs/ctu-other.c
@@ -12,11 +12,11 @@
 }
 
 // Test enums.
-enum B { x = 42,
- l,
- s };
+enum B { x2 = 42,
+ y2,
+ z2 };
 int enumCheck(void) {
-  return x;
+  return x2;
 }
 
 // Test reporting an error in macro definition
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -79,6 +79,7 @@
   using ExpectedExpr = llvm::Expected;
   using ExpectedDecl = llvm::Expected;
   using ExpectedSLoc = llvm::Expected;
+  using ExpectedName = llvm::Expected;
 
   std::string ImportError::toString() const {
 // FIXME: Improve error texts.
@@ -1944,15 +1945,6 @@
 
 bool ASTNodeImporter::IsStructuralMatch(RecordDecl *FromRecord,
 RecordDecl *ToRecord, bool Complain) {
-  // Eliminate a potential failure point where we attempt to re-import
-  // something we're trying to import while completing ToRecord.
-  Decl *ToOrigin = Importer.GetOriginalDecl(ToRecord);
-  if (ToOrigin) {
-auto *ToOriginRecord = dyn_cast(ToOrigin);
-if (ToOriginRecord)
-  ToRecord = ToOriginRecord;
-  }
-
   StructuralEquivalenceContext Ctx(Importer.getFromContext(),
ToRecord->getASTContext(),
Importer.getNonEquivalentDecls(),
@@ -2154,11 +2146,11 @@
 }
 
 if (!ConflictingDecls.empty()) {
-  Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Namespace,
- ConflictingDecls.data(),
- ConflictingDecls.size());
-  if (!Name)
-return 

[PATCH] D59692: [ASTImporter] Fix name conflict handling

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:2260
 
   ConflictingDecls.push_back(FoundDecl);
 }

a_sidorin wrote:
> Do we push the same decl twice?
Uhh, thanks, that is rebase error I've made.



Comment at: lib/AST/ASTImporter.cpp:8532
 unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
 }

a_sidorin wrote:
> Empty DeclarationName can be valid sometimes. Should we return 
> ErrorOr instead? This can also simplify caller code a bit.
That's a good idea, thanks.
Though, I'd use Expected rather, that would be more consistent with the rest 
of the code. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D59692



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

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D59485#1444673 , @teemperor wrote:

> In D59485#1439628 , @martong wrote:
>
> > In D59485#1439570 , @martong wrote:
> >
> > > In D59485#1439390 , @teemperor 
> > > wrote:
> > >
> > > > > Well, I still don't understand how LLDB synthesis the AST. 
> > > > >  So you have a C++ module in a .pcm file. This means you could create 
> > > > > an AST from that with ASTReader from it's .clang_ast section, right? 
> > > > > In that case the AST should be complete with all type information. If 
> > > > > this was the case then I don't see why it is not possible to use 
> > > > > clang::ASTImporter to import templates and specializations, since we 
> > > > > do exactly that in CTU.
> > > > > 
> > > > > Or do you guys create the AST from the debug info, from the 
> > > > > .debug_info section of .pcm module file? And this is why AST is 
> > > > > incomplete? I've got this from 
> > > > > https://youtu.be/EgkZ8PTNSHQ?list=PL85Cf-ok7HpppFWlp2wX_-A1dkyFVlaEB=1565
> > > > >  If this is the case, then comes my naiive quiestion: Why don't you 
> > > > > use the ASTReader?
> > > >
> > > > There are no C++ modules involved in our use case beside the generic 
> > > > `std` module. No user-written code is read from a module and we don't 
> > > > have any PCM file beside the `std` module we build for the expression 
> > > > evaluator.
> > > >
> > > > We use the ASTReader to directly read the `std` module contents into 
> > > > the expression evaluation ASTContext, but this doesn't give us the 
> > > > decls for the instantiations the user has made (e.g. 
> > > > `std::vector`). We only have these user instantiations in the 
> > > > 'normal' debug info where we have a rather minimal AST (again, no 
> > > > modules are not involved in building this debug info AST). The 
> > > > `InternalImport` in the LLDB patch just stitches the module AST and the 
> > > > debug info AST together when we import something that we also have (in 
> > > > better quality from the C++ module) in the target ASTContext.
> > >
> > >
> > > Thank you for the explanation.
> > >  Now I understand you directly create specializations from the std module 
> > > and intercept the import to avoid importing broken specializations from 
> > > the debug info, this makes sense.
> >
> >
> > Really, just one last question to see the whole picture: If 
> > clang::ASTImporter were capable of handling a specialization/instantiation 
> > with missing info then we could use that. So the reason for this 
> > interception is that 
> > clang::ASTImporter::VisitClassTemplateSpecializationDecl cannot handle the 
> > specialization it receives because that or its dependent nodes has too many 
> > missing data, right?
>
>
> Feel free to ask! I'm just traveling so my internet access (and my reply 
> rate) is a bit low at the moment.
>
> Not sure if we can easily merge the logic of D59537 
>  into the ASTImporter. It has no logic at 
> all to actually determine if a source AST node has missing data but solely 
> relies on our knowledge that our AST in LLDB isn't very useful for std 
> templates. Also instantiating a template instead of importing an existing 
> instantiation seems like a very rare scenario. I'm not even sure how we would 
> test having a broken AST with Clang (the parser won't produce one, so we 
> would have to hack our own AST builder for broken nodes).
>
> If there is a reasonable way to get this logic into the ASTImporter I have no 
> objection against doing so. The only problem I see is that I can't come up 
> with a reasonable way of merging/testing the logic in the ASTImporter (and 
> what other application would benefit from it).


Raphael, thank you for your answer. This explanation makes so much more easier 
for me to understand the need for this patch.


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

https://reviews.llvm.org/D59485



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

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:590
+  new RedirectingImporter(ToContext, ToFileManager, FromContext,
+  FromFileManager, MinimalImport, LookupTabl));
+};

teemperor wrote:
> a_sidorin wrote:
> > LookupTable?
> Not sure if I can follow?
I think Alexey's comment relates to the parameter in 
`ASTImporterOptionSpecificTestBase::ImporterConstructor`. There is a typo in 
the name of the parameter an 'e' is missing: `ASTImporterLookupTable 
*LookupTabl`


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

https://reviews.llvm.org/D59485



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

[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D59665#1446764 , @a_sidorin wrote:

> Hi Shafik,
>  Thank you for the explanation, it is much more clear to me now. But, as I 
> see, D59692  is going to discard the changes 
> this patch introduces. @martong Gabor, do you expect the changes of this 
> patch to be merged into yours, or should this patch be abandoned?


Actually, I'll have to adjust D59692  to not 
discard these changes (otherwise we might end up a regression in lldb).


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

https://reviews.llvm.org/D59665



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

[PATCH] D59485: [ASTImporter] Add an ImportInternal method to allow customizing Import behavior.

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

In D59485#1446950 , @a_sidorin wrote:

> Hello Raphael,
>  I think we should accept this change. I don't see an easy way to merge the 
> LLDB stuff into ASTImporter; also, this patch provides a good extension point 
> for ASTImporter since it is designed to be a parent class. @martong @shafik 
> Gabor, Shafik, what do you think?


Yes, I agree.


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

https://reviews.llvm.org/D59485



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

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357402: [ASTImporter] Convert ODR diagnostics inside 
ASTImporter implementation (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59761

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2961,7 +2961,7 @@
   continue;
 
 // Complain about inconsistent function types.
-Importer.ToDiag(Loc, diag::err_odr_function_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_function_type_inconsistent)
 << Name << D->getType() << FoundFunction->getType();
 Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
 << FoundFunction->getType();
@@ -3265,7 +3265,7 @@
   }
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3336,7 +3336,7 @@
 continue;
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3467,7 +3467,7 @@
 return FoundIvar;
   }
 
-  Importer.ToDiag(Loc, diag::err_odr_ivar_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_ivar_type_inconsistent)
 << Name << D->getType() << FoundIvar->getType();
   Importer.ToDiag(FoundIvar->getLocation(), diag::note_odr_value_here)
 << FoundIvar->getType();
@@ -3580,7 +3580,7 @@
   }
 }
 
-Importer.ToDiag(Loc, diag::err_odr_variable_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_variable_type_inconsistent)
   << Name << D->getType() << FoundVar->getType();
 Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
   << FoundVar->getType();
@@ -3745,7 +3745,7 @@
   // Check return types.
   if (!Importer.IsStructurallyEquivalent(D->getReturnType(),
  FoundMethod->getReturnType())) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_result_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_result_type_inconsistent)
 << D->isInstanceMethod() << Name << D->getReturnType()
 << FoundMethod->getReturnType();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3757,7 +3757,7 @@
 
   // Check the number of parameters.
   if (D->param_size() != FoundMethod->param_size()) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_num_params_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_num_params_inconsistent)
   << D->isInstanceMethod() << Name
   << D->param_size() << FoundMethod->param_size();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3774,7 +3774,7 @@
 if (!Importer.IsStructurallyEquivalent((*P)->getType(),
(*FoundP)->getType())) {
   Importer.FromDiag((*P)->getLocation(),
-diag::err_odr_objc_method_param_type_inconsistent)
+diag::warn_odr_objc_method_param_type_inconsistent)
 << D->isInstanceMethod() << Name
 << (*P)->getType() << (*FoundP)->getType();
   Importer.ToDiag((*FoundP)->getLocation(), diag::note_odr_value_here)
@@ -3787,7 +3787,7 @@
   // Check variadic/non-variadic.
   // Check the number of parameters.
   if (D->isVariadic() != FoundMethod->isVariadic()) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_variadic_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_variadic_inconsistent)
   << D->isInstanceMethod() << Name;
 Importer.ToDiag(FoundMethod->getLocation(),
 diag::note_odr_objc_method_here)
@@ -4332,7 +4332,7 @@
 if ((bool)FromSuper != (bool)ToSuper ||
 (FromSuper && !declaresSameEntity(FromSuper, ToSuper))) {
   Importer.ToDiag(To->getLocation(),
-  diag::err_odr_objc_superclass_inconsistent)
+  diag::warn_odr_objc_superclass_inconsistent)
 << To->getDeclName();
   if 

[PATCH] D58897: [ASTImporter] Make ODR error handling configurable

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357394: [ASTImporter] Make ODR error handling configurable 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58897?vs=192070=193086#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58897

Files:
  include/clang/AST/ASTStructuralEquivalence.h
  include/clang/Basic/DiagnosticASTKinds.td
  include/clang/Basic/DiagnosticGroups.td
  lib/AST/ASTStructuralEquivalence.cpp

Index: include/clang/AST/ASTStructuralEquivalence.h
===
--- include/clang/AST/ASTStructuralEquivalence.h
+++ include/clang/AST/ASTStructuralEquivalence.h
@@ -111,6 +111,10 @@
   static llvm::Optional
   findUntaggedStructOrUnionIndex(RecordDecl *Anon);
 
+  // If ErrorOnTagTypeMismatch is set, return the the error, otherwise get the
+  // relevant warning for the input error diagnostic.
+  unsigned getApplicableDiagnostic(unsigned ErrorDiagnostic);
+
 private:
   /// Finish checking all of the structural equivalences.
   ///
Index: include/clang/Basic/DiagnosticASTKinds.td
===
--- include/clang/Basic/DiagnosticASTKinds.td
+++ include/clang/Basic/DiagnosticASTKinds.td
@@ -224,20 +224,31 @@
 def err_odr_variable_type_inconsistent : Error<
   "external variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_variable_type_inconsistent : Warning<
+  "external variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_variable_multiple_def : Error<
   "external variable %0 defined in multiple translation units">;
+def warn_odr_variable_multiple_def : Warning<
+  "external variable %0 defined in multiple translation units">,
+  InGroup;
 def note_odr_value_here : Note<"declared here with type %0">;
 def note_odr_defined_here : Note<"also defined here">;
 def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
-def warn_odr_tag_type_inconsistent
-: Warning<"type %0 has incompatible definitions in different translation "
-  "units">,
-  InGroup>;
+def warn_odr_function_type_inconsistent : Warning<
+  "external function %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_tag_type_inconsistent
 : Error<"type %0 has incompatible definitions in different translation "
 "units">;
+def warn_odr_tag_type_inconsistent
+: Warning<"type %0 has incompatible definitions in different translation "
+  "units">,
+  InGroup;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;
@@ -253,44 +264,82 @@
   "class has %0 base %plural{1:class|:classes}0">;
 def note_odr_enumerator : Note<"enumerator %0 with value %1 here">;
 def note_odr_missing_enumerator : Note<"no corresponding enumerator here">;
-
 def err_odr_field_type_inconsistent : Error<
   "field %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_field_type_inconsistent : Warning<
+  "field %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 
 // Importing Objective-C ASTs
 def err_odr_ivar_type_inconsistent : Error<
   "instance variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_ivar_type_inconsistent : Warning<
+  "instance variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_objc_superclass_inconsistent : Error<
   "class %0 has incompatible superclasses">;
+def warn_odr_objc_superclass_inconsistent : Warning<
+  "class %0 has incompatible superclasses">,
+  InGroup;
 def note_odr_objc_superclass : Note<"inherits from superclass %0 here">;
 def note_odr_objc_missing_superclass : Note<"no corresponding superclass here">;
 def err_odr_objc_method_result_type_inconsistent : Error<
   "%select{class|instance}0 method %1 has incompatible result types in "
   "different translation units (%2 vs. %3)">;
+def warn_odr_objc_method_result_type_inconsistent : Warning<
+  "%select{class|instance}0 method %1 has incompatible result types in "
+  "different translation units (%2 vs. %3)">,
+  InGroup;
 def err_odr_objc_method_num_params_inconsistent : Error<
   "%select{class|instance}0 method %1 has a different number of parameters in "
   "different translation units (%2 vs. %3)">;
+def warn_odr_objc_method_num_params_inconsistent : Warning<
+  "%select{class|instance}0 method %1 has a different number of parameters in "
+  "different 

[PATCH] D58663: [ASTImporter] Add support for importing ChooseExpr AST nodes.

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:1344
+
+  EXPECT_EQ(FromChooseExpr->isConditionTrue(), 
ToChooseExpr->isConditionTrue());
+}

To  compensate the  skipping of the template test, perhaps we should have 
another expectation for the condition dependency of the expression:
```
EXPECT_EQ(FromChooseExpr->isConditionDependent(), 
ToChooseExpr->isConditionDependent());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58663



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

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

Redecl chains of function template specializations are not handled well
currently. We want to handle them similarly to functions, i.e. try to
keep the structure of the original AST as much as possible. The aim is
to not squash a prototype with a definition, rather we create both and
put them in a redecl chain.


Repository:
  rC Clang

https://reviews.llvm.org/D58668

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3859,6 +3859,47 @@
   std::string getDefinition() { return TypeParam::Definition; }
   BindableMatcher getPattern() const { return TypeParam().getPattern(); }
 
+  void CheckPreviousDecl(Decl *To0, Decl *To1) {
+ASSERT_NE(To0, To1);
+ASSERT_EQ(>getASTContext(), >getASTContext());
+
+auto *ToTU = To0->getTranslationUnitDecl();
+
+// Templates.
+if (auto *ToT0 = dyn_cast(To0)) {
+  EXPECT_EQ(To1->getPreviousDecl(), To0);
+  auto *ToT1 = cast(To1);
+  ASSERT_TRUE(ToT0->getTemplatedDecl());
+  ASSERT_TRUE(ToT1->getTemplatedDecl());
+  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
+ToT0->getTemplatedDecl());
+  return;
+}
+
+// Specializations.
+if (auto *From0F = dyn_cast(To0)) {
+  auto *To0F = cast(To0);
+  if (From0F->getTemplatedKind() ==
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+EXPECT_EQ(To0->getCanonicalDecl(), To1->getCanonicalDecl());
+// There may be a hidden fwd spec decl before a spec decl.
+// In that case the previous visible decl can be reached through that
+// invisible one.
+EXPECT_THAT(To0,
+testing::AnyOf(To1->getPreviousDecl(),
+   To1->getPreviousDecl()->getPreviousDecl()));
+auto *TemplateD = FirstDeclMatcher().match(
+ToTU, functionTemplateDecl());
+auto *FirstSpecD = *(TemplateD->spec_begin());
+EXPECT_EQ(FirstSpecD->getCanonicalDecl(), To0F->getCanonicalDecl());
+return;
+  }
+}
+
+// The rest: Classes, Functions, etc.
+EXPECT_EQ(To1->getPreviousDecl(), To0);
+  }
+
   void
   TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition() {
 Decl *FromTU = getTuDecl(getPrototype(), Lang_CXX);
@@ -3911,14 +3952,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
+
+CheckPreviousDecl(To0, To1);
   }
 
   void TypedTest_ImportDefinitionAfterImportedPrototype() {
@@ -3940,14 +3975,8 @@
 EXPECT_TRUE(ImportedDef == ToDef);
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
-EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
-if (auto *ToProtoT = dyn_cast(ToProto)) {
-  auto *ToDefT = cast(ToDef);
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  EXPECT_EQ(ToDefT->getTemplatedDecl()->getPreviousDecl(),
-ToProtoT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToProto, ToDef);
   }
 
   void TypedTest_ImportPrototypeAfterImportedDefinition() {
@@ -3969,14 +3998,8 @@
 EXPECT_TRUE(ImportedProto == ToProto);
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
-EXPECT_EQ(ToProto->getPreviousDecl(), ToDef);
-if (auto *ToDefT = dyn_cast(ToDef)) {
-  auto *ToProtoT = cast(ToProto);
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  EXPECT_EQ(ToProtoT->getTemplatedDecl()->getPreviousDecl(),
-ToDefT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToDef, ToProto);
   }
 
   void TypedTest_ImportPrototypes() {
@@ -3998,27 +4021,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-

[PATCH] D57590: [ASTImporter] Improve import of FileID.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a reviewer: martong.

Ping.

Please raise your objections if you have any until the 4th of March (that date 
we are going to commit if there are no objections). Also, please let us know if 
you find this deadline too strict.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57590



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4966
 // it has any definition in the redecl chain.
-static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
-  CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+template  static auto getDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();

a_sidorin wrote:
> We should point that this function is for TemplateDecls only somehow. But we 
> can't just pass TemplateDecl as the parameter due to loss of the actual 
> return type. Maybewe should rename this function into 
> "getTemplateDefinition()"?
Ok, I changed that to `getTemplateDefinition`.



Comment at: lib/AST/ASTImporter.cpp:5563
+  // TODO: handle conflicting names
+} // linkage
+  }   // template

a_sidorin wrote:
> We don't usually put such comments after control flow statements. If they are 
> really needed, it is a good sign that a function must be split, and it's 
> better to leave a FIXME for this (or do the split).
Ok, I removed these comments and added a FIXME.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 188379.
martong marked 4 inline comments as done.
martong added a comment.

- getDefinition -> getTemplateDefinition
- Remove comments for braces, added FIXME


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -4163,9 +4163,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, ,
@@ -4180,9 +4179,8 @@
 RedeclChain, Class, , DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, , DefinitionShouldBeImportedAsADefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, , DefinitionShouldBeImportedAsADefinition)
@@ -4196,9 +4194,7 @@
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypeAfterImportedPrototype)
@@ -4212,9 +4208,7 @@
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitionAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportDefinitionAfterImportedPrototype)
 // FIXME This does not pass, possible error with ClassTemplate import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
@@ -4229,9 +4223,7 @@
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedDefinition)
 // FIXME This does not pass, possible error with ClassTemplate import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
@@ -4244,9 +4236,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, , ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypes)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_, ImportPrototypes)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
+ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypes)
 // FIXME This does not pass, possible error with Spec import.
@@ -4259,9 +4250,8 @@
 ImportDefinitions)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitions)
-// FIXME Enable this test, once we import function templates chains 

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: a_sidorin, shafik.
Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp, 
rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.

Redecl chains of class template specializations are not handled well
currently. We want to handle them similarly to functions, i.e. try to
keep the structure of the original AST as much as possible. The aim is
to not squash a prototype with a definition, rather we create both and
put them in a redecl chain.


Repository:
  rC Clang

https://reviews.llvm.org/D58673

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3471,12 +3471,10 @@
   // The second specialization is different from the first, thus it violates
   // ODR, consequently we expect to keep the first specialization only, which is
   // already in the "To" context.
-  EXPECT_TRUE(ImportedSpec);
-  auto *ToSpec = FirstDeclMatcher().match(
-  ToTU, classTemplateSpecializationDecl(hasName("X")));
-  EXPECT_EQ(ImportedSpec, ToSpec);
-  EXPECT_EQ(1u, DeclCounter().match(
-ToTU, classTemplateSpecializationDecl()));
+  EXPECT_FALSE(ImportedSpec);
+  EXPECT_EQ(1u,
+DeclCounter().match(
+ToTU, classTemplateSpecializationDecl(hasName("X";
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
@@ -3851,6 +3849,23 @@
   }
 };
 
+struct ClassTemplateSpec {
+  using DeclTy = ClassTemplateSpecializationDecl;
+  static constexpr auto *Prototype =
+R"(
+template  class X;
+template <> class X;
+)";
+  static constexpr auto *Definition =
+R"(
+template  class X;
+template <> class X {};
+)";
+  BindableMatcher getPattern() {
+return classTemplateSpecializationDecl(hasName("X"), unless(isImplicit()));
+  }
+};
+
 template 
 struct RedeclChain : ASTImporterOptionSpecificTestBase {
 
@@ -4173,6 +4188,9 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, FunctionTemplateSpec, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
+RedeclChain, ClassTemplateSpec, ,
+PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Function, , DefinitionShouldBeImportedAsADefinition)
@@ -4189,6 +4207,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, FunctionTemplateSpec, ,
 DefinitionShouldBeImportedAsADefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
+RedeclChain, ClassTemplateSpec, , DefinitionShouldBeImportedAsADefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedPrototype)
@@ -4204,6 +4224,8 @@
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportPrototypeAfterImportedPrototype)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypeAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportDefinitionAfterImportedPrototype)
@@ -4221,6 +4243,8 @@
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportDefinitionAfterImportedPrototype)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportDefinitionAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedDefinition)
@@ -4238,6 +4262,8 @@
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportPrototypeAfterImportedDefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypeAfterImportedDefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypes)
@@ -4252,6 +4278,8 @@
 // FIXME This does not pass, possible error with Spec import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec,
 DISABLED_, ImportPrototypes)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypes)
 
 

[PATCH] D58743: Handle built-in when importing SourceLocation and FileID

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a reviewer: balazske.
martong added a subscriber: balazske.
martong added a comment.

Shafik, this looks good to me, once teemperor's comments are addressed.
Note, I added @balazske as a reviewer, he recently worked with importing the 
FileIDs, he may have some comments.


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

https://reviews.llvm.org/D58743



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

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik, Could you please take a look?
I have run the LLDB tests on our macOS and I could not discover any regression.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58502



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik, Could you please take a look?

I have run the LLDB tests on our macOS and I could not discover any regression.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.
Closed by commit rC355096: [CTU] Do not allow different CPP dialects in CTU 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D57906?vs=185926=188740#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D57906

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp


Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
   "requested function's body");
 STATISTIC(NumTripleMismatch, "The # of triple mismatches");
 STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
 
 // Same as Triple's equality operator, but we check a field only if that is
 // known in both instances.
@@ -99,6 +100,8 @@
   return "Triple mismatch";
 case index_error_code::lang_mismatch:
   return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+  return "Language dialect mismatch";
 }
 llvm_unreachable("Unrecognized index_error_code.");
   }
@@ -228,6 +231,7 @@
 
   const auto  = Context.getLangOpts();
   const auto  = Unit->getASTContext().getLangOpts();
+
   // FIXME: Currenty we do not support CTU across C++ and C and across
   // different dialects of C++.
   if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
 return llvm::make_error(index_error_code::lang_mismatch);
   }
 
+  // If CPP dialects are different then return with error.
+  //
+  // Consider this STL code:
+  //   template
+  // struct __alloc_traits
+  //   #if __cplusplus >= 201103L
+  // : std::allocator_traits<_Alloc>
+  //   #endif
+  // { // ...
+  // };
+  // This class template would create ODR errors during merging the two units,
+  // since in one translation unit the class template has a base class, however
+  // in the other unit it has none.
+  if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+  LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+  LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+  LangTo.CPlusPlus2a != LangFrom.CPlusPlus2a) {
+++NumLangDialectMismatch;
+return llvm::make_error(
+index_error_code::lang_dialect_mismatch);
+  }
+
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
   if (const FunctionDecl *ResultDecl =
   findFunctionInDeclContext(TU, LookupFnName))
Index: include/clang/CrossTU/CrossTranslationUnit.h
===
--- include/clang/CrossTU/CrossTranslationUnit.h
+++ include/clang/CrossTU/CrossTranslationUnit.h
@@ -43,7 +43,8 @@
   failed_to_get_external_ast,
   failed_to_generate_usr,
   triple_mismatch,
-  lang_mismatch
+  lang_mismatch,
+  lang_dialect_mismatch
 };
 
 class IndexError : public llvm::ErrorInfo {


Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -42,6 +42,7 @@
   "requested function's body");
 STATISTIC(NumTripleMismatch, "The # of triple mismatches");
 STATISTIC(NumLangMismatch, "The # of language mismatches");
+STATISTIC(NumLangDialectMismatch, "The # of language dialect mismatches");
 
 // Same as Triple's equality operator, but we check a field only if that is
 // known in both instances.
@@ -99,6 +100,8 @@
   return "Triple mismatch";
 case index_error_code::lang_mismatch:
   return "Language mismatch";
+case index_error_code::lang_dialect_mismatch:
+  return "Language dialect mismatch";
 }
 llvm_unreachable("Unrecognized index_error_code.");
   }
@@ -228,6 +231,7 @@
 
   const auto  = Context.getLangOpts();
   const auto  = Unit->getASTContext().getLangOpts();
+
   // FIXME: Currenty we do not support CTU across C++ and C and across
   // different dialects of C++.
   if (LangTo.CPlusPlus != LangFrom.CPlusPlus) {
@@ -235,6 +239,28 @@
 return llvm::make_error(index_error_code::lang_mismatch);
   }
 
+  // If CPP dialects are different then return with error.
+  //
+  // Consider this STL code:
+  //   template
+  // struct __alloc_traits
+  //   #if __cplusplus >= 201103L
+  // : std::allocator_traits<_Alloc>
+  //   #endif
+  // { // ...
+  // };
+  // This class template would create ODR errors during merging the two units,
+  // since in one translation unit the class template has a base class, however
+  // in the other unit it has none.
+  if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+  LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||
+  LangTo.CPlusPlus17 != LangFrom.CPlusPlus17 ||
+  

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 189130.
martong marked 4 inline comments as done.
martong added a comment.

- Add FindAndMapDefinition as member fun
- Refactor CheckPreviousDecl


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3859,6 +3859,45 @@
   std::string getDefinition() { return TypeParam::Definition; }
   BindableMatcher getPattern() const { return TypeParam().getPattern(); }
 
+  void CheckPreviousDecl(Decl *Prev, Decl *Current) {
+ASSERT_NE(Prev, Current);
+ASSERT_EQ(>getASTContext(), >getASTContext());
+EXPECT_EQ(Prev->getCanonicalDecl(), Current->getCanonicalDecl());
+
+// Templates.
+if (auto *PrevT = dyn_cast(Prev)) {
+  EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  auto *CurrentT = cast(Current);
+  ASSERT_TRUE(PrevT->getTemplatedDecl());
+  ASSERT_TRUE(CurrentT->getTemplatedDecl());
+  EXPECT_EQ(CurrentT->getTemplatedDecl()->getPreviousDecl(),
+PrevT->getTemplatedDecl());
+  return;
+}
+
+// Specializations.
+if (auto *PrevF = dyn_cast(Prev)) {
+  if (PrevF->getTemplatedKind() ==
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+// There may be a hidden fwd spec decl before a spec decl.
+// In that case the previous visible decl can be reached through that
+// invisible one.
+EXPECT_THAT(Prev, testing::AnyOf(
+  Current->getPreviousDecl(),
+  Current->getPreviousDecl()->getPreviousDecl()));
+auto *ToTU = Prev->getTranslationUnitDecl();
+auto *TemplateD = FirstDeclMatcher().match(
+ToTU, functionTemplateDecl());
+auto *FirstSpecD = *(TemplateD->spec_begin());
+EXPECT_EQ(FirstSpecD->getCanonicalDecl(), PrevF->getCanonicalDecl());
+return;
+  }
+}
+
+// The rest: Classes, Functions, etc.
+EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  }
+
   void
   TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition() {
 Decl *FromTU = getTuDecl(getPrototype(), Lang_CXX);
@@ -3911,14 +3950,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
+
+CheckPreviousDecl(To0, To1);
   }
 
   void TypedTest_ImportDefinitionAfterImportedPrototype() {
@@ -3940,14 +3973,8 @@
 EXPECT_TRUE(ImportedDef == ToDef);
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
-EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
-if (auto *ToProtoT = dyn_cast(ToProto)) {
-  auto *ToDefT = cast(ToDef);
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  EXPECT_EQ(ToDefT->getTemplatedDecl()->getPreviousDecl(),
-ToProtoT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToProto, ToDef);
   }
 
   void TypedTest_ImportPrototypeAfterImportedDefinition() {
@@ -3969,14 +3996,8 @@
 EXPECT_TRUE(ImportedProto == ToProto);
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
-EXPECT_EQ(ToProto->getPreviousDecl(), ToDef);
-if (auto *ToDefT = dyn_cast(ToDef)) {
-  auto *ToProtoT = cast(ToProto);
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  EXPECT_EQ(ToProtoT->getTemplatedDecl()->getPreviousDecl(),
-ToDefT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToDef, ToProto);
   }
 
   void TypedTest_ImportPrototypes() {
@@ -3998,27 +4019,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
-// Extra check for specializations.
-// FIXME Add this check to other tests too (possibly factor out into a
-// function), when they start to pass.
-if (auto *From0F = dyn_cast(From0)) {
-  auto *To0F = cast(To0);
-

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong marked 8 inline comments as done.
martong added a comment.

Alexei, thanks for the review!




Comment at: lib/AST/ASTImporter.cpp:3002
 
+  // Check if we have found an existing definition.  Returns with that
+  // definition if yes, otherwise returns null.

a_sidorin wrote:
> I like this lambda. To make the code even better, we can move this lambda 
> outside of VisitFunctionDecl because this method is already pretty big.
Ok, I have created a member function in ASTNodeImporter instead of the lambda.



Comment at: unittests/AST/ASTImporterTest.cpp:3862
 
+  void CheckPreviousDecl(Decl *To0, Decl *To1) {
+ASSERT_NE(To0, To1);

a_sidorin wrote:
> I don't like numbers. Maybe `To0` and `To1` are `LastDecl` and 
> `ImportedDecl`, correspondingly?
Ok, I have renamed To0 to Prev and To1 to Current as they should form a redecl 
chain this way: Prev->Current.



Comment at: unittests/AST/ASTImporterTest.cpp:3881
+if (auto *From0F = dyn_cast(To0)) {
+  auto *To0F = cast(To0);
+  if (From0F->getTemplatedKind() ==

a_sidorin wrote:
> To0 and From0F actually have the same value, and To0F is unused.
Good catch. I removed the second variable.



Comment at: unittests/AST/ASTImporterTest.cpp:3884
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+EXPECT_EQ(To0->getCanonicalDecl(), To1->getCanonicalDecl());
+// There may be a hidden fwd spec decl before a spec decl.

a_sidorin wrote:
> If I don't miss something, this assumption does not depend on To0 and To1 
> kind and can be moved out of the condition, to the function scope.
Yes, thanks for catching this. I have moved this upward right at the beginning 
of the function.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

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

- Fix some comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3471,12 +3471,10 @@
   // The second specialization is different from the first, thus it violates
   // ODR, consequently we expect to keep the first specialization only, which is
   // already in the "To" context.
-  EXPECT_TRUE(ImportedSpec);
-  auto *ToSpec = FirstDeclMatcher().match(
-  ToTU, classTemplateSpecializationDecl(hasName("X")));
-  EXPECT_EQ(ImportedSpec, ToSpec);
-  EXPECT_EQ(1u, DeclCounter().match(
-ToTU, classTemplateSpecializationDecl()));
+  EXPECT_FALSE(ImportedSpec);
+  EXPECT_EQ(1u,
+DeclCounter().match(
+ToTU, classTemplateSpecializationDecl(hasName("X";
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
@@ -3851,6 +3849,23 @@
   }
 };
 
+struct ClassTemplateSpec {
+  using DeclTy = ClassTemplateSpecializationDecl;
+  static constexpr auto *Prototype =
+R"(
+template  class X;
+template <> class X;
+)";
+  static constexpr auto *Definition =
+R"(
+template  class X;
+template <> class X {};
+)";
+  BindableMatcher getPattern() {
+return classTemplateSpecializationDecl(hasName("X"), unless(isImplicit()));
+  }
+};
+
 template 
 struct RedeclChain : ASTImporterOptionSpecificTestBase {
 
@@ -4173,6 +4188,9 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, FunctionTemplateSpec, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
+RedeclChain, ClassTemplateSpec, ,
+PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Function, , DefinitionShouldBeImportedAsADefinition)
@@ -4189,6 +4207,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, FunctionTemplateSpec, ,
 DefinitionShouldBeImportedAsADefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
+RedeclChain, ClassTemplateSpec, , DefinitionShouldBeImportedAsADefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedPrototype)
@@ -4204,6 +4224,8 @@
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportPrototypeAfterImportedPrototype)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypeAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportDefinitionAfterImportedPrototype)
@@ -4221,6 +4243,8 @@
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportDefinitionAfterImportedPrototype)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportDefinitionAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedDefinition)
@@ -4238,6 +4262,8 @@
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportPrototypeAfterImportedDefinition)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypeAfterImportedDefinition)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypes)
@@ -4252,6 +4278,8 @@
 // FIXME This does not pass, possible error with Spec import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec,
 DISABLED_, ImportPrototypes)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplateSpec, ,
+ImportPrototypes)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportDefinitions)
@@ -4267,6 +4295,8 @@
 // FIXME This does not pass, possible error with Spec import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec,
 DISABLED_, ImportDefinitions)

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Alexei, thanks for the review!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

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

Redecl chains of function templates are not handled well currently. We
want to handle them similarly to functions, i.e. try to keep the
structure of the original AST as much as possible. The aim is to not
squash a prototype with a definition, rather we create both and put them
in a redecl chain.


Repository:
  rC Clang

https://reviews.llvm.org/D58494

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -4163,9 +4163,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, ,
@@ -4180,9 +4179,8 @@
 RedeclChain, Class, , DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, , DefinitionShouldBeImportedAsADefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, , DefinitionShouldBeImportedAsADefinition)
@@ -4196,9 +4194,7 @@
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypeAfterImportedPrototype)
@@ -4212,9 +4208,7 @@
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitionAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportDefinitionAfterImportedPrototype)
 // FIXME This does not pass, possible error with ClassTemplate import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
@@ -4229,9 +4223,7 @@
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedDefinition)
 // FIXME This does not pass, possible error with ClassTemplate import.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
@@ -4244,9 +4236,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, , ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypes)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_, ImportPrototypes)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
+ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypes)
 // FIXME This does not pass, possible error with Spec import.
@@ -4259,9 +4250,8 @@
   

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: a_sidorin, shafik.
Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp, 
rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.

Redecl chains of classes and class templates are not handled well
currently. We want to handle them similarly to functions, i.e. try to
keep the structure of the original AST as much as possible. The aim is
to not squash a prototype with a definition, rather we create both and
put them in a redecl chain.


Repository:
  rC Clang

https://reviews.llvm.org/D58502

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -4207,8 +4207,7 @@
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportDefinitionAfterImportedPrototype)
-// FIXME This does not pass, possible error with Class import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, ,
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitionAfterImportedPrototype)
@@ -4216,16 +4215,14 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
 DISABLED_,
 ImportDefinitionAfterImportedPrototype)
-// FIXME This does not pass, possible error with ClassTemplate import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportDefinitionAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedDefinition)
-// FIXME This does not pass, possible error with Class import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, ,
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedDefinition)
@@ -4233,8 +4230,7 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
 DISABLED_,
 ImportPrototypeAfterImportedDefinition)
-// FIXME This does not pass, possible error with ClassTemplate import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportPrototypeAfterImportedDefinition)
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2552,26 +2552,6 @@
 Decl::FOK_None;
   }
 
-  // If this record has a definition in the translation unit we're coming from,
-  // but this particular declaration is not that definition, import the
-  // definition and map to that.
-  TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D &&
-  // Friend template declaration must be imported on its own.
-  !IsFriendTemplate &&
-  // In contrast to a normal CXXRecordDecl, the implicit
-  // CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration.
-  // The definition of the implicit CXXRecordDecl in this case is the
-  // ClassTemplateSpecializationDecl itself. Thus, we start with an extra
-  // condition in order to be able to import the implict Decl.
-  !D->isImplicit()) {
-ExpectedDecl ImportedDefOrErr = import(Definition);
-if (!ImportedDefOrErr)
-  return ImportedDefOrErr.takeError();
-
-return Importer.MapImported(D, *ImportedDefOrErr);
-  }
-
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
@@ -2600,7 +2580,8 @@
 auto FoundDecls =
 Importer.findDeclsInToCtx(DC, SearchName);
 if (!FoundDecls.empty()) {
-  // We're going to have to compare D against potentially conflicting Decls, so complete it.
+  // We're going to have to compare D against potentially 

[PATCH] D58502: [ASTImporter] Fix redecl failures of Class and ClassTemplate

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355390: [ASTImporter] Fix redecl failures of Class and 
ClassTemplate (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58502

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2553,26 +2553,6 @@
 Decl::FOK_None;
   }
 
-  // If this record has a definition in the translation unit we're coming from,
-  // but this particular declaration is not that definition, import the
-  // definition and map to that.
-  TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D &&
-  // Friend template declaration must be imported on its own.
-  !IsFriendTemplate &&
-  // In contrast to a normal CXXRecordDecl, the implicit
-  // CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration.
-  // The definition of the implicit CXXRecordDecl in this case is the
-  // ClassTemplateSpecializationDecl itself. Thus, we start with an extra
-  // condition in order to be able to import the implict Decl.
-  !D->isImplicit()) {
-ExpectedDecl ImportedDefOrErr = import(Definition);
-if (!ImportedDefOrErr)
-  return ImportedDefOrErr.takeError();
-
-return Importer.MapImported(D, *ImportedDefOrErr);
-  }
-
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
@@ -2601,7 +2581,8 @@
 auto FoundDecls =
 Importer.findDeclsInToCtx(DC, SearchName);
 if (!FoundDecls.empty()) {
-  // We're going to have to compare D against potentially conflicting Decls, so complete it.
+  // We're going to have to compare D against potentially conflicting Decls,
+  // so complete it.
   if (D->hasExternalLexicalStorage() && !D->isCompleteDefinition())
 D->getASTContext().getExternalSource()->CompleteType(D);
 }
@@ -4976,17 +4957,6 @@
 ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
   bool IsFriend = D->getFriendObjectKind() != Decl::FOK_None;
 
-  // If this template has a definition in the translation unit we're coming
-  // from, but this particular declaration is not that definition, import the
-  // definition and map to that.
-  ClassTemplateDecl *Definition = getDefinition(D);
-  if (Definition && Definition != D && !IsFriend) {
-if (ExpectedDecl ImportedDefOrErr = import(Definition))
-  return Importer.MapImported(D, *ImportedDefOrErr);
-else
-  return ImportedDefOrErr.takeError();
-  }
-
   // Import the major distinguishing characteristics of this class template.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -4241,8 +4241,7 @@
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportDefinitionAfterImportedPrototype)
-// FIXME This does not pass, possible error with Class import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, ,
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitionAfterImportedPrototype)
@@ -4250,16 +4249,14 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
 DISABLED_,
 ImportDefinitionAfterImportedPrototype)
-// FIXME This does not pass, possible error with ClassTemplate import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplateSpec, ,
 ImportDefinitionAfterImportedPrototype)
 
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Function, ,
 ImportPrototypeAfterImportedDefinition)
-// FIXME This does not pass, possible error with Class import.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, ,
 

[PATCH] D58830: [ASTImporter] Import member expr with explicit template args

[Gabor Marton via Phabricator via cfe-commits]

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

Member expressions with explicit template arguments were not imported
correctly: the DeclRefExpr was missing. This patch fixes.


Repository:
  rC Clang

https://reviews.llvm.org/D58830

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2604,6 +2604,56 @@
   EXPECT_TRUE(LambdaRec->getDestructor());
 }
 
+TEST_P(ImportFunctions,
+   CallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  void f() {
+X x;
+x.foo();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  EXPECT_TRUE(MatchVerifier().match(
+  ToD, functionDecl(hasName("f"), hasDescendant(declRefExpr();
+}
+
+TEST_P(ImportFunctions,
+   DependentCallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  template 
+  void f() {
+X x;
+x.foo();
+  }
+  void g() {
+f();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("g")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  EXPECT_TRUE(MatchVerifier().match(
+  ToTU, translationUnitDecl(hasDescendant(
+functionDecl(hasName("f"), hasDescendant(declRefExpr()));
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -7135,15 +7135,19 @@
 
   DeclarationNameInfo ToMemberNameInfo(ToName, ToLoc);
 
+  TemplateArgumentListInfo ToTAInfo, *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-// FIXME: handle template arguments
-return make_error(ImportError::UnsupportedConstruct);
+if (Error Err =
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))
+  return std::move(Err);
+ResInfo = 
   }
 
   return MemberExpr::Create(
   Importer.getToContext(), ToBase, E->isArrow(), ToOperatorLoc,
   ToQualifierLoc, ToTemplateKeywordLoc, ToMemberDecl, ToFoundDecl,
-  ToMemberNameInfo, nullptr, ToType, E->getValueKind(), 
E->getObjectKind());
+  ToMemberNameInfo, ResInfo, ToType, E->getValueKind(), 
E->getObjectKind());
 }
 
 ExpectedStmt


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2604,6 +2604,56 @@
   EXPECT_TRUE(LambdaRec->getDestructor());
 }
 
+TEST_P(ImportFunctions,
+   CallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  void f() {
+X x;
+x.foo();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  EXPECT_TRUE(MatchVerifier().match(
+  ToD, functionDecl(hasName("f"), hasDescendant(declRefExpr();
+}
+
+TEST_P(ImportFunctions,
+   DependentCallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  template 
+  void f() {
+X x;
+x.foo();
+  }
+  void g() {
+f();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("g")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  EXPECT_TRUE(MatchVerifier().match(
+  ToTU, translationUnitDecl(hasDescendant(
+functionDecl(hasName("f"), hasDescendant(declRefExpr()));
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ 

[PATCH] D57906: [CTU] Do not allow different CPP dialects in CTU

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:255
+  // in the other unit it has none.
+  if (LangTo.CPlusPlus11 != LangFrom.CPlusPlus11 ||
+  LangTo.CPlusPlus14 != LangFrom.CPlusPlus14 ||

r.stahl wrote:
> This is likely to become a bug in the future, but I didn't see a better way 
> to compare dialects.
> 
> Is there a way to add a test that lights up once there is a new dialect?
> 
> Would it be too pessimistic to compare the entire LangOpts? Some stuff in 
> there should even still produce errors, right? For example "GnuMode". I 
> skimmed over them and didn't find an obvious one that would differ between 
> translation units of the same project. Maybe the template depth could be set 
> selectively, but to fix that we could mask "COMPATIBLE_" and "BENIGN_" opts.
> This is likely to become a bug in the future, but I didn't see a better way 
> to compare dialects.
> Is there a way to add a test that lights up once there is a new dialect?

`LANGOPTS` are defined as bitfields: `#define LANGOPT(Name, Bits, Default, 
Description) unsigned Name : Bits;`
I can't think of any solution to avoid the future bug, because there is no way 
to enumerate all the C++ dialects.
I am going to ask around about this on cfe-dev, maybe somebody will have an 
idea.

> Would it be too pessimistic to compare the entire LangOpts?
I think that would be too pessimistic.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57906



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

[PATCH] D57590: [ASTImporter] Improve import of FileID.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D57590#1411055 , @efriedma wrote:

> I don't know anything about this particular patch, but you aren't allowed to 
> set deadlines like that; the patch review process requires that the patch is 
> actually reviewed.  If you aren't getting a response, ask on cfe-dev.


Okay sorry about that, I will not commit until we get an approve. I think I was 
mislead by the "LLVM Developer Policy" 
(https://llvm.org/docs/DeveloperPolicy.html)

> Do not assume silent approval, or request active objections to the patch with 
> a deadline.

I interpreted this as after several pings setting a deadline is okay, 
especially if the patch is small and getting old.
Perhaps the policy should be updated to avoid such misinterpretation in the 
future.

Unfortunately this part of Clang (ASTImporter) has quite a few active 
developers other than my colleges. Obviously my colleges could review these 
patches (and they have done that in our fork) but we thought it would be better 
to have accept from devs of other organizations. One of the clients of 
ASTImporter is cross translation unit (CTU) static analysis, the other is LLDB. 
We hope to have more users and developers of CTU by reaching a point where it 
is mature enough to attract more developers. At the moment CTU analysis is not 
successful even on a simple C project like tmux with the upstream master. But 
that is successful on complex C++ projects like protobuf since a long time in 
our fork. Upstreaming to master takes painfully too much time. Our fork is 
already ahead at least 25 commits and it is growing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57590



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@shafik Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added a comment.

@shafik Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355593: [ASTImporter] Handle redecl chain of 
FunctionTemplateDecls (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58494?vs=189665=189694#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58494

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4943,15 +4943,15 @@
   return ToD;
 }
 
-// Returns the definition for a (forward) declaration of a ClassTemplateDecl, if
+// Returns the definition for a (forward) declaration of a TemplateDecl, if
 // it has any definition in the redecl chain.
-static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
-  CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+template  static auto getTemplateDefinition(T *D) -> T * {
+  assert(D->getTemplatedDecl() && "Should be called on templates only");
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)
 return nullptr;
-  ClassTemplateDecl *TemplateWithDef =
-  ToTemplatedDef->getDescribedClassTemplate();
-  return TemplateWithDef;
+  auto *TemplateWithDef = ToTemplatedDef->getDescribedTemplate();
+  return cast_or_null(TemplateWithDef);
 }
 
 ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
@@ -4983,7 +4983,8 @@
   if (FoundTemplate) {
 
 if (IsStructuralMatch(D, FoundTemplate)) {
-  ClassTemplateDecl *TemplateWithDef = getDefinition(FoundTemplate);
+  ClassTemplateDecl *TemplateWithDef =
+  getTemplateDefinition(FoundTemplate);
   if (D->isThisDeclarationADefinition() && TemplateWithDef) {
 return Importer.MapImported(D, TemplateWithDef);
   }
@@ -5046,6 +5047,8 @@
 // and this time the lookup finds the previous fwd friend class template.
 // In this case we must set up the previous decl for the templated decl.
 if (!ToTemplated->getPreviousDecl()) {
+  assert(FoundByLookup->getTemplatedDecl() &&
+ "Found decl must have its templated decl set");
   CXXRecordDecl *PrevTemplated =
   FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
   if (ToTemplated != PrevTemplated)
@@ -5508,8 +5511,11 @@
   if (ToD)
 return ToD;
 
+  const FunctionTemplateDecl *FoundByLookup = nullptr;
+
   // Try to find a function in our own ("to") context with the same name, same
   // type, and in the same context as the function we're importing.
+  // FIXME Split this into a separate function.
   if (!LexicalDC->isFunctionOrMethod()) {
 unsigned IDNS = Decl::IDNS_Ordinary | Decl::IDNS_OrdinaryFriend;
 auto FoundDecls = Importer.findDeclsInToCtx(DC, Name);
@@ -5517,18 +5523,21 @@
   if (!FoundDecl->isInIdentifierNamespace(IDNS))
 continue;
 
-  if (auto *FoundFunction =
-  dyn_cast(FoundDecl)) {
-if (FoundFunction->hasExternalFormalLinkage() &&
+  if (auto *FoundTemplate = dyn_cast(FoundDecl)) {
+if (FoundTemplate->hasExternalFormalLinkage() &&
 D->hasExternalFormalLinkage()) {
-  if (IsStructuralMatch(D, FoundFunction)) {
-Importer.MapImported(D, FoundFunction);
-// FIXME: Actually try to merge the body and other attributes.
-return FoundFunction;
+  if (IsStructuralMatch(D, FoundTemplate)) {
+FunctionTemplateDecl *TemplateWithDef =
+getTemplateDefinition(FoundTemplate);
+if (D->isThisDeclarationADefinition() && TemplateWithDef) {
+  return Importer.MapImported(D, TemplateWithDef);
+}
+FoundByLookup = FoundTemplate;
+break;
   }
+  // TODO: handle conflicting names
 }
   }
-  // TODO: handle conflicting names
 }
   }
 
@@ -5547,10 +5556,25 @@
 return ToFunc;
 
   TemplatedFD->setDescribedFunctionTemplate(ToFunc);
+
   ToFunc->setAccess(D->getAccess());
   ToFunc->setLexicalDeclContext(LexicalDC);
-
   LexicalDC->addDeclInternal(ToFunc);
+
+  if (FoundByLookup) {
+auto *Recent =
+const_cast(FoundByLookup->getMostRecentDecl());
+if (!TemplatedFD->getPreviousDecl()) {
+  assert(FoundByLookup->getTemplatedDecl() &&
+ "Found decl must have its templated decl set");
+  auto *PrevTemplated =
+  FoundByLookup->getTemplatedDecl()->getMostRecentDecl();
+  if (TemplatedFD != PrevTemplated)
+TemplatedFD->setPreviousDecl(PrevTemplated);
+}
+ToFunc->setPreviousDecl(Recent);
+  }
+
   return ToFunc;
 }
 
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ 

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a reviewer: martong.
Herald added a project: clang.

Ping. This patch is the next in the series of patches to finish the proper 
error handling (i.e. using Error and Expected). Please see the "Stack" 
column above.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53757



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

[PATCH] D58830: [ASTImporter] Import member expr with explicit template args

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL355596: [ASTImporter] Import member expr with explicit 
template args (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58830

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -7129,15 +7129,19 @@
 
   DeclarationNameInfo ToMemberNameInfo(ToName, ToLoc);
 
+  TemplateArgumentListInfo ToTAInfo, *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-// FIXME: handle template arguments
-return make_error(ImportError::UnsupportedConstruct);
+if (Error Err =
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), 
E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))
+  return std::move(Err);
+ResInfo = 
   }
 
   return MemberExpr::Create(
   Importer.getToContext(), ToBase, E->isArrow(), ToOperatorLoc,
   ToQualifierLoc, ToTemplateKeywordLoc, ToMemberDecl, ToFoundDecl,
-  ToMemberNameInfo, nullptr, ToType, E->getValueKind(), 
E->getObjectKind());
+  ToMemberNameInfo, ResInfo, ToType, E->getValueKind(), 
E->getObjectKind());
 }
 
 ExpectedStmt
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -2604,6 +2604,56 @@
   EXPECT_TRUE(LambdaRec->getDestructor());
 }
 
+TEST_P(ImportFunctions,
+   CallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  void f() {
+X x;
+x.foo();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  EXPECT_TRUE(MatchVerifier().match(
+  ToD, functionDecl(hasName("f"), hasDescendant(declRefExpr();
+}
+
+TEST_P(ImportFunctions,
+   DependentCallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  template 
+  void f() {
+X x;
+x.foo();
+  }
+  void g() {
+f();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("g")));
+  auto *ToD = Import(FromD, Lang_CXX);
+  EXPECT_TRUE(ToD);
+  Decl *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  EXPECT_TRUE(MatchVerifier().match(
+  ToTU, translationUnitDecl(hasDescendant(
+functionDecl(hasName("f"), hasDescendant(declRefExpr()));
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -7129,15 +7129,19 @@
 
   DeclarationNameInfo ToMemberNameInfo(ToName, ToLoc);
 
+  TemplateArgumentListInfo ToTAInfo, *ResInfo = nullptr;
   if (E->hasExplicitTemplateArgs()) {
-// FIXME: handle template arguments
-return make_error(ImportError::UnsupportedConstruct);
+if (Error Err =
+ImportTemplateArgumentListInfo(E->getLAngleLoc(), E->getRAngleLoc(),
+   E->template_arguments(), ToTAInfo))
+  return std::move(Err);
+ResInfo = 
   }
 
   return MemberExpr::Create(
   Importer.getToContext(), ToBase, E->isArrow(), ToOperatorLoc,
   ToQualifierLoc, ToTemplateKeywordLoc, ToMemberDecl, ToFoundDecl,
-  ToMemberNameInfo, nullptr, ToType, E->getValueKind(), E->getObjectKind());
+  ToMemberNameInfo, ResInfo, ToType, E->getValueKind(), E->getObjectKind());
 }
 
 ExpectedStmt
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -2604,6 +2604,56 @@
   EXPECT_TRUE(LambdaRec->getDestructor());
 }
 
+TEST_P(ImportFunctions,
+   CallExprOfMemberFunctionTemplateWithExplicitTemplateArgs) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct X {
+template 
+void foo(){}
+  };
+  void f() {
+X x;
+x.foo();
+  }
+  )",
+  Lang_CXX);
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("f")));
+  auto *ToD = 

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTImporter.cpp:5147
   }
+} else { // ODR violation.
+  // FIXME HandleNameConflict

shafik wrote:
> ODR violations are ill-formed no diagnostic required. So currently will this 
> fail for cases that clang proper would not?
> ODR violations are ill-formed no diagnostic required.

ASTStructuralEquivalenceContext already provides diagnostic in the ODR cases. 
E.g.:
```
  // Check for equivalent field names.
  IdentifierInfo *Name1 = Field1->getIdentifier();
  IdentifierInfo *Name2 = Field2->getIdentifier();
  if (!::IsStructurallyEquivalent(Name1, Name2)) {
if (Context.Complain) {
  Context.Diag2(Owner2->getLocation(),
Context.ErrorOnTagTypeMismatch
? diag::err_odr_tag_type_inconsistent
: diag::warn_odr_tag_type_inconsistent)
  << Context.ToCtx.getTypeDeclType(Owner2);
  Context.Diag2(Field2->getLocation(), diag::note_odr_field_name)
  << Field2->getDeclName();
  Context.Diag1(Field1->getLocation(), diag::note_odr_field_name)
  << Field1->getDeclName();
}
return false;
  }
```
We change this to be always a Warning instead of an Error in this patch: 
https://reviews.llvm.org/D58897

> So currently will this fail for cases that clang proper would not?

Well, I think the situation is more subtle than that.
There are cases when we can link two translation units and the linker provides 
a valid executable even if there is an ODR violation of a type. There is no 
type information used during linkage, except for functions' mangled names and 
visibility. That ODR violation is recognized though when we do the merge in the 
AST level (and I think it is useful to diagnose them).
So if "clang proper" includes the linker then the answer is yes. If not then we 
are talking about only one translation unit and the answer is no.



Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 189665.
martong marked 6 inline comments as done.
martong added a comment.

- Add asserts


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -4197,9 +4197,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, ,
@@ -4214,9 +4213,8 @@
 RedeclChain, Class, , DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, Variable, , DefinitionShouldBeImportedAsADefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
-RedeclChain, FunctionTemplate, DISABLED_,
+RedeclChain, FunctionTemplate, ,
 DefinitionShouldBeImportedAsADefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(
 RedeclChain, ClassTemplate, , DefinitionShouldBeImportedAsADefinition)
@@ -4230,9 +4228,7 @@
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypeAfterImportedPrototype)
@@ -4245,9 +4241,7 @@
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitionAfterImportedPrototype)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportDefinitionAfterImportedPrototype)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportDefinitionAfterImportedPrototype)
@@ -4260,9 +4254,7 @@
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypeAfterImportedDefinition)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_,
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
 ImportPrototypeAfterImportedDefinition)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypeAfterImportedDefinition)
@@ -4274,9 +4266,8 @@
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Class, , ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportPrototypes)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate,
-DISABLED_, ImportPrototypes)
+ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, FunctionTemplate, ,
+ImportPrototypes)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, ClassTemplate, ,
 ImportPrototypes)
 // FIXME This does not pass, possible error with Spec import.
@@ -4289,9 +4280,8 @@
 ImportDefinitions)
 ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, Variable, ,
 ImportDefinitions)
-// FIXME Enable this test, once we import function templates chains correctly.
-ASTIMPORTER_INSTANTIATE_TYPED_TEST_CASE(RedeclChain, 

[PATCH] D58494: [ASTImporter] Handle redecl chain of FunctionTemplateDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4967
+template  static auto getTemplateDefinition(T *D) -> T * {
+  auto *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
   if (!ToTemplatedDef)

shafik wrote:
> shafik wrote:
> > Can we guarantee that `D->getTemplatedDecl()` will always return a valid 
> > pointer? 
> What other types besides `CXXRecordDecl` do we expect here? 
> Can we guarantee that D->getTemplatedDecl() will always return a valid 
> pointer?

Actually, we always call this function on a `ClassTemplateDecl` or on a 
`FunctionTemplateDecl` which should return with a valid pointer. Still it is a 
good idea to put an assert at the beginning of the function, so I just did that.
```
  assert(D->getTemplatedDecl() && "Should be called on templates only");
```

> What other types besides CXXRecordDecl do we expect here?

`FunctionDecl` can be there too when we call this on a `FunctiomTemplateDecl` 
param.




Comment at: lib/AST/ASTImporter.cpp:5544
   // type, and in the same context as the function we're importing.
+  // FIXME Split this into a separate function.
   if (!LexicalDC->isFunctionOrMethod()) {

shafik wrote:
> Would it make sense to do the split into a separate function in the PR?
I'd rather do that in another patch where we could address the similar issues 
in other visit functions too. Actually, most of the visit functions start with 
a lookup and then continue with a loop over the lookup results; this part could 
be split into a separate function everywhere.



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

shafik wrote:
> Can we guarantee that `FoundByLookup->getTemplatedDecl()` will always return 
> a valid pointer? 
This is a good catch, thanks.
The `FoundByLookup` variable's type is `FunctionTemplateDecl`. Either the found 
Decl is created by the parser then it must have set up a templated decl, or it 
is created by the ASTImporter and we should have a templated decl set.
In the second case, however, we may not be 100% sure that the templated is 
really set.
So I have added an assert here too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58494



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668



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

[PATCH] D58897: [ASTImporter] Make ODR error handling configurable

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D58897



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

[PATCH] D55358: [ASTImporter] Fix import of NestedNameSpecifierLoc.

[Gabor Marton via Phabricator via cfe-commits]

martong added a reviewer: a_sidorin.
martong added a comment.
Herald added a reviewer: martong.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D55358



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

[PATCH] D53757: [ASTImporter] Changed use of Import to Import_New in ASTNodeImporter.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a subscriber: rnkovacs.

> This patch is the next in the series of patches to finish the proper error 
> handling (i.e. using Error and Expected). Please see the "Stack" column 
> above.

@balazske Or is it superseded by this one? https://reviews.llvm.org/D53818


Repository:
  rC Clang

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

https://reviews.llvm.org/D53757



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

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 189830.
martong edited the summary of this revision.
martong added a comment.
Herald added a project: clang.

Rebase to master.
There was a conflict in the tests, in ASTImporterTest.cpp.


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1537,7 +1537,7 @@
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
-   DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+   CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   // The original recursive algorithm of ASTImporter first imports 'c' then
@@ -5517,5 +5517,16 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
 DefaultTestValuesForRunOptions, );
 
+TEST_P(ImportDecl, ImportFieldOrder) {
+  MatchVerifier Verifier;
+  testImport("struct declToImport {"
+ "  int b = a + 2;"
+ "  int a = 5;"
+ "};",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ recordDecl(hasFieldOrder({"b", "a"})));
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1635,13 +1635,61 @@
 auto ToDCOrErr = Importer.ImportContext(FromDC);
 return ToDCOrErr.takeError();
   }
-  llvm::SmallVector ImportedDecls;
+
+  const auto *FromRD = dyn_cast(FromDC);
   for (auto *From : FromDC->decls()) {
 ExpectedDecl ImportedOrErr = import(From);
-if (!ImportedOrErr)
+if (!ImportedOrErr) {
+  // For RecordDecls, failed import of a field will break the layout of the
+  // structure. Handle it as an error.
+  if (FromRD)
+return ImportedOrErr.takeError();
   // Ignore the error, continue with next Decl.
   // FIXME: Handle this case somehow better.
-  consumeError(ImportedOrErr.takeError());
+  else
+consumeError(ImportedOrErr.takeError());
+}
+  }
+
+  // Reorder declarations in RecordDecls because they may have another
+  // order. Keeping field order is vitable because it determines structure
+  // layout.
+  // FIXME: This is an ugly fix. Unfortunately, I cannot come with better
+  // solution for this issue. We cannot defer expression import here because
+  // type import can depend on them.
+  if (!FromRD)
+return Error::success();
+
+
+  // NOTE: Here and below, we cannot call field_begin() method and its callers
+  // on ToRD if it has an external storage. Calling field_begin() will
+  // automatically load all the fields by calling
+  // LoadFieldsFromExternalStorage().
+  auto ImportedDC = import(cast(FromDC));
+  assert(ImportedDC);
+  auto *ToRD = cast(*ImportedDC);
+  for (auto *D : FromRD->decls()) {
+if (isa(D) || isa(D)) {
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));
+  ToRD->removeDecl(ToD);
+}
+  }
+
+  if (!ToRD->hasExternalLexicalStorage())
+assert(ToRD->field_empty());
+
+  for (auto *D : FromRD->decls()) {
+if (isa(D) || isa(D)) {
+  Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+  assert(ToD);
+  assert(ToRD == ToD->getDeclContext());
+  assert(ToRD == ToD->getLexicalDeclContext());
+  if (!ToRD->hasExternalLexicalStorage())
+assert(!ToRD->containsDecl(ToD));
+
+  ToRD->addDeclInternal(ToD);
+}
   }
 
   return Error::success();


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1537,7 +1537,7 @@
 }
 
 TEST_P(ASTImporterOptionSpecificTestBase,
-   DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
+   CXXRecordDeclFieldOrderShouldNotDependOnImportOrder) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   // The original recursive algorithm of ASTImporter first imports 'c' then
@@ -5517,5 +5517,16 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
 DefaultTestValuesForRunOptions, );
 
+TEST_P(ImportDecl, ImportFieldOrder) {
+  MatchVerifier Verifier;
+  testImport("struct declToImport {"
+ "  int b = a + 2;"
+ "  int a = 5;"
+ "};",
+ Lang_CXX11, "", Lang_CXX11, Verifier,
+ recordDecl(hasFieldOrder({"b", "a"})));
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/ASTImporter.cpp
===
--- 

[PATCH] D44100: [ASTImporter] Reorder fields after structure import is finished

[Gabor Marton via Phabricator via cfe-commits]

martong commandeered this revision.
martong edited reviewers, added: a_sidorin; removed: martong.
martong added a comment.
This revision now requires review to proceed.
Herald added subscribers: gamesh411, dkrupp.

@a_sidorin Aleksei, If you don't mind, I am going to investigate this further 
with macOS/LLDB and try to answer Adrian's comments too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D44100



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

[PATCH] D59595: Remove the unused return value in ASTImporter::Imported [NFC]

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D59595



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

[PATCH] D59485: [ASTImporter] Allow adding a import strategy to the ASTImporter

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

> We can't reliable import templates with the ASTImporter, so actually 
> reconstructing the template in the debug info AST and then importing it 
> doesn't work.

Could you please elaborate how the import of templates fails in ASTImporter? Is 
it because the AST you build from the DWARF is incomplete? Or is there a lookup 
problem? Is there an assertion in one of the CodeGen functions once the import 
is finished?
If we could fix that error in the ASTImporter then other clients of it (like 
CTU) could benefit from the fix too. Perhaps, the best would be to have a test 
which reproduces the template import related errors, maybe a lit test with 
clang-import-test?

If the crux of the problem is because of the incomplete AST from DWARF then 
probably we cannot fix that here in clang::ASTImporter. However, if that is not 
the case then I think we should try to fix the error here instead of 
duplicating lookup and import logic in LLDB. (As for the lookup problems, quite 
recently I just  have discovered, that lookup during expression evaluation in 
LLDB does not work properly, I'll communicate this in a separate email/patch.)

> It would also be much slower.

Could you please explain why it would be slower?


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

https://reviews.llvm.org/D59485



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

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356455: [ASTImporter] Fix redecl failures of 
FunctionTemplateSpec (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58668

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -290,6 +290,16 @@
 ToD->setImplicit();
 }
 
+// Check if we have found an existing definition.  Returns with that
+// definition if yes, otherwise returns null.
+Decl *FindAndMapDefinition(FunctionDecl *D, FunctionDecl *FoundFunction) {
+  const FunctionDecl *Definition = nullptr;
+  if (D->doesThisDeclarationHaveABody() &&
+  FoundFunction->hasBody(Definition))
+return Importer.MapImported(D, const_cast(Definition));
+  return nullptr;
+}
+
   public:
 explicit ASTNodeImporter(ASTImporter ) : Importer(Importer) {}
 
@@ -2973,8 +2983,8 @@
 if (!FoundFunctionOrErr)
   return FoundFunctionOrErr.takeError();
 if (FunctionDecl *FoundFunction = *FoundFunctionOrErr) {
-  if (D->doesThisDeclarationHaveABody() && FoundFunction->hasBody())
-return Importer.MapImported(D, FoundFunction);
+  if (Decl *Def = FindAndMapDefinition(D, FoundFunction))
+return Def;
   FoundByLookup = FoundFunction;
 }
   }
@@ -2993,11 +3003,8 @@
   continue;
 
 if (IsStructuralMatch(D, FoundFunction)) {
-  const FunctionDecl *Definition = nullptr;
-  if (D->doesThisDeclarationHaveABody() &&
-  FoundFunction->hasBody(Definition))
-return Importer.MapImported(D,
-const_cast(Definition));
+  if (Decl *Def = FindAndMapDefinition(D, FoundFunction))
+return Def;
   FoundByLookup = FoundFunction;
   break;
 }
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3978,6 +3978,45 @@
   std::string getDefinition() { return TypeParam::Definition; }
   BindableMatcher getPattern() const { return TypeParam().getPattern(); }
 
+  void CheckPreviousDecl(Decl *Prev, Decl *Current) {
+ASSERT_NE(Prev, Current);
+ASSERT_EQ(>getASTContext(), >getASTContext());
+EXPECT_EQ(Prev->getCanonicalDecl(), Current->getCanonicalDecl());
+
+// Templates.
+if (auto *PrevT = dyn_cast(Prev)) {
+  EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  auto *CurrentT = cast(Current);
+  ASSERT_TRUE(PrevT->getTemplatedDecl());
+  ASSERT_TRUE(CurrentT->getTemplatedDecl());
+  EXPECT_EQ(CurrentT->getTemplatedDecl()->getPreviousDecl(),
+PrevT->getTemplatedDecl());
+  return;
+}
+
+// Specializations.
+if (auto *PrevF = dyn_cast(Prev)) {
+  if (PrevF->getTemplatedKind() ==
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+// There may be a hidden fwd spec decl before a spec decl.
+// In that case the previous visible decl can be reached through that
+// invisible one.
+EXPECT_THAT(Prev, testing::AnyOf(
+  Current->getPreviousDecl(),
+  Current->getPreviousDecl()->getPreviousDecl()));
+auto *ToTU = Prev->getTranslationUnitDecl();
+auto *TemplateD = FirstDeclMatcher().match(
+ToTU, functionTemplateDecl());
+auto *FirstSpecD = *(TemplateD->spec_begin());
+EXPECT_EQ(FirstSpecD->getCanonicalDecl(), PrevF->getCanonicalDecl());
+return;
+  }
+}
+
+// The rest: Classes, Functions, etc.
+EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  }
+
   void
   TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition() {
 Decl *FromTU = getTuDecl(getPrototype(), Lang_CXX);
@@ -4030,14 +4069,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
+
+CheckPreviousDecl(To0, To1);
   }
 
   void TypedTest_ImportDefinitionAfterImportedPrototype() {
@@ -4059,14 +4092,8 @@
 EXPECT_TRUE(ImportedDef == ToDef);
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
 

[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356452: [ASTImporter] Fix redecl failures of 
ClassTemplateSpec (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58673?vs=189131=191285#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58673

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -5052,17 +5052,6 @@
 
 ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(
   ClassTemplateSpecializationDecl *D) {
-  // If this record has a definition in the translation unit we're coming from,
-  // but this particular declaration is not that definition, import the
-  // definition and map to that.
-  TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
-if (ExpectedDecl ImportedDefOrErr = import(Definition))
-  return Importer.MapImported(D, *ImportedDefOrErr);
-else
-  return ImportedDefOrErr.takeError();
-  }
-
   ClassTemplateDecl *ClassTemplate;
   if (Error Err = importInto(ClassTemplate, D->getSpecializedTemplate()))
 return std::move(Err);
@@ -5080,154 +5069,141 @@
 
   // Try to find an existing specialization with these template arguments.
   void *InsertPos = nullptr;
-  ClassTemplateSpecializationDecl *D2 = nullptr;
+  ClassTemplateSpecializationDecl *PrevDecl = nullptr;
   ClassTemplatePartialSpecializationDecl *PartialSpec =
 dyn_cast(D);
   if (PartialSpec)
-D2 = ClassTemplate->findPartialSpecialization(TemplateArgs, InsertPos);
+PrevDecl =
+ClassTemplate->findPartialSpecialization(TemplateArgs, InsertPos);
   else
-D2 = ClassTemplate->findSpecialization(TemplateArgs, InsertPos);
-  ClassTemplateSpecializationDecl * const PrevDecl = D2;
-  RecordDecl *FoundDef = D2 ? D2->getDefinition() : nullptr;
-  if (FoundDef) {
-if (!D->isCompleteDefinition()) {
-  // The "From" translation unit only had a forward declaration; call it
-  // the same declaration.
-  // TODO Handle the redecl chain properly!
-  return Importer.MapImported(D, FoundDef);
-}
-
-if (IsStructuralMatch(D, FoundDef)) {
-
-  Importer.MapImported(D, FoundDef);
-
-  // Import those those default field initializers which have been
-  // instantiated in the "From" context, but not in the "To" context.
-  for (auto *FromField : D->fields()) {
-auto ToOrErr = import(FromField);
-if (!ToOrErr)
-  // FIXME: return the error?
-  consumeError(ToOrErr.takeError());
-  }
+PrevDecl = ClassTemplate->findSpecialization(TemplateArgs, InsertPos);
 
-  // Import those methods which have been instantiated in the
-  // "From" context, but not in the "To" context.
-  for (CXXMethodDecl *FromM : D->methods()) {
-auto ToOrErr = import(FromM);
-if (!ToOrErr)
-  // FIXME: return the error?
-  consumeError(ToOrErr.takeError());
+  if (PrevDecl) {
+if (IsStructuralMatch(D, PrevDecl)) {
+  if (D->isThisDeclarationADefinition() && PrevDecl->getDefinition()) {
+Importer.MapImported(D, PrevDecl->getDefinition());
+// Import those default field initializers which have been
+// instantiated in the "From" context, but not in the "To" context.
+for (auto *FromField : D->fields())
+  Importer.Import(FromField);
+
+// Import those methods which have been instantiated in the
+// "From" context, but not in the "To" context.
+for (CXXMethodDecl *FromM : D->methods())
+  Importer.Import(FromM);
+
+// TODO Import instantiated default arguments.
+// TODO Import instantiated exception specifications.
+//
+// Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
+// what else could be fused during an AST merge.
+return PrevDecl;
   }
+} else { // ODR violation.
+  // FIXME HandleNameConflict
+  return nullptr;
+}
+  }
 
-  // TODO Import instantiated default arguments.
-  // TODO Import instantiated exception specifications.
-  //
-  // Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint what
-  // else could be fused during an AST merge.
-
-  return FoundDef;
-}
-  } else { // We either couldn't find any previous specialization in the "To"
-   // context,  or we found one but without definition.  Let's create a
-   // new specialization and register that at the class template.
+  // Import the location of this declaration.
+  ExpectedSLoc BeginLocOrErr = 

[PATCH] D58668: [ASTImporter] Fix redecl failures of FunctionTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

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

- Rebase to master


Repository:
  rC Clang

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

https://reviews.llvm.org/D58668

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3978,6 +3978,45 @@
   std::string getDefinition() { return TypeParam::Definition; }
   BindableMatcher getPattern() const { return TypeParam().getPattern(); }
 
+  void CheckPreviousDecl(Decl *Prev, Decl *Current) {
+ASSERT_NE(Prev, Current);
+ASSERT_EQ(>getASTContext(), >getASTContext());
+EXPECT_EQ(Prev->getCanonicalDecl(), Current->getCanonicalDecl());
+
+// Templates.
+if (auto *PrevT = dyn_cast(Prev)) {
+  EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  auto *CurrentT = cast(Current);
+  ASSERT_TRUE(PrevT->getTemplatedDecl());
+  ASSERT_TRUE(CurrentT->getTemplatedDecl());
+  EXPECT_EQ(CurrentT->getTemplatedDecl()->getPreviousDecl(),
+PrevT->getTemplatedDecl());
+  return;
+}
+
+// Specializations.
+if (auto *PrevF = dyn_cast(Prev)) {
+  if (PrevF->getTemplatedKind() ==
+  FunctionDecl::TK_FunctionTemplateSpecialization) {
+// There may be a hidden fwd spec decl before a spec decl.
+// In that case the previous visible decl can be reached through that
+// invisible one.
+EXPECT_THAT(Prev, testing::AnyOf(
+  Current->getPreviousDecl(),
+  Current->getPreviousDecl()->getPreviousDecl()));
+auto *ToTU = Prev->getTranslationUnitDecl();
+auto *TemplateD = FirstDeclMatcher().match(
+ToTU, functionTemplateDecl());
+auto *FirstSpecD = *(TemplateD->spec_begin());
+EXPECT_EQ(FirstSpecD->getCanonicalDecl(), PrevF->getCanonicalDecl());
+return;
+  }
+}
+
+// The rest: Classes, Functions, etc.
+EXPECT_EQ(Current->getPreviousDecl(), Prev);
+  }
+
   void
   TypedTest_PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition() {
 Decl *FromTU = getTuDecl(getPrototype(), Lang_CXX);
@@ -4030,14 +4069,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
+
+CheckPreviousDecl(To0, To1);
   }
 
   void TypedTest_ImportDefinitionAfterImportedPrototype() {
@@ -4059,14 +4092,8 @@
 EXPECT_TRUE(ImportedDef == ToDef);
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
-EXPECT_EQ(ToDef->getPreviousDecl(), ToProto);
-if (auto *ToProtoT = dyn_cast(ToProto)) {
-  auto *ToDefT = cast(ToDef);
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  EXPECT_EQ(ToDefT->getTemplatedDecl()->getPreviousDecl(),
-ToProtoT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToProto, ToDef);
   }
 
   void TypedTest_ImportPrototypeAfterImportedDefinition() {
@@ -4088,14 +4115,8 @@
 EXPECT_TRUE(ImportedProto == ToProto);
 EXPECT_TRUE(ToDef->isThisDeclarationADefinition());
 EXPECT_FALSE(ToProto->isThisDeclarationADefinition());
-EXPECT_EQ(ToProto->getPreviousDecl(), ToDef);
-if (auto *ToDefT = dyn_cast(ToDef)) {
-  auto *ToProtoT = cast(ToProto);
-  ASSERT_TRUE(ToDefT->getTemplatedDecl());
-  ASSERT_TRUE(ToProtoT->getTemplatedDecl());
-  EXPECT_EQ(ToProtoT->getTemplatedDecl()->getPreviousDecl(),
-ToDefT->getTemplatedDecl());
-}
+
+CheckPreviousDecl(ToDef, ToProto);
   }
 
   void TypedTest_ImportPrototypes() {
@@ -4117,27 +4138,8 @@
 EXPECT_TRUE(Imported1 == To1);
 EXPECT_FALSE(To0->isThisDeclarationADefinition());
 EXPECT_FALSE(To1->isThisDeclarationADefinition());
-EXPECT_EQ(To1->getPreviousDecl(), To0);
-if (auto *ToT0 = dyn_cast(To0)) {
-  auto *ToT1 = cast(To1);
-  ASSERT_TRUE(ToT0->getTemplatedDecl());
-  ASSERT_TRUE(ToT1->getTemplatedDecl());
-  EXPECT_EQ(ToT1->getTemplatedDecl()->getPreviousDecl(),
-ToT0->getTemplatedDecl());
-}
-// Extra check for specializations.
-// FIXME Add this check to other tests too (possibly factor out into a
-// function), when they start to pass.
-if (auto *From0F = dyn_cast(From0)) {
-  auto *To0F = cast(To0);
-  if (From0F->getTemplatedKind() ==
-