[PATCH] D49792: [ASTmporter] SourceRange-free function parameter checking for declarations

2018-08-06 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339018: [ASTmporter] SourceRange-free function parameter 
checking for declarations (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49792?vs=157236&id=159303#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49792

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
@@ -1147,15 +1147,21 @@
   FunctionDecl *FunDecl;
   if (isa(D) && (FunDecl = dyn_cast(OrigDC)) &&
   FunDecl->hasBody()) {
-SourceRange RecR = D->getSourceRange();
-SourceRange BodyR = FunDecl->getBody()->getSourceRange();
-// If RecordDecl is not in Body (it is a param), we bail out.
-if (RecR.isValid() && BodyR.isValid() &&
-(RecR.getBegin() < BodyR.getBegin() ||
- BodyR.getEnd() < RecR.getEnd())) {
-  Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
-  << D->getDeclKindName();
-  return true;
+auto getLeafPointeeType = [](const Type *T) {
+  while (T->isPointerType() || T->isArrayType()) {
+T = T->getPointeeOrArrayElementType();
+  }
+  return T;
+};
+for (const ParmVarDecl *P : FunDecl->parameters()) {
+  const Type *LeafT =
+  getLeafPointeeType(P->getType().getCanonicalType().getTypePtr());
+  auto *RT = dyn_cast(LeafT);
+  if (RT && RT->getDecl() == D) {
+Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
+<< D->getDeclKindName();
+return true;
+  }
 }
   }
 
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -989,7 +989,7 @@
"  return 0;"
"}",
Lang_C, "input.c");
-  auto FromVar =
+  auto *FromVar =
   FirstDeclMatcher().match(FromTU, varDecl(hasName("d")));
   ASSERT_TRUE(FromVar);
   auto ToType =
@@ -999,12 +999,41 @@
 
 TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncParams) {
   // This construct is not supported by ASTImporter.
-  Decl *FromTU =
-  getTuDecl("int declToImport(struct data_t{int a;int b;} *d){ return 0; 
}",
-Lang_C, "input.c");
-  auto From = FirstDeclMatcher().match(FromTU, functionDecl());
+  Decl *FromTU = getTuDecl(
+  "int declToImport(struct data_t{int a;int b;} ***d){ return 0; }",
+  Lang_C, "input.c");
+  auto *From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  ASSERT_TRUE(From);
+  auto *To = Import(From, Lang_C);
+  EXPECT_EQ(To, nullptr);
+}
+
+TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncFromMacro) {
+  Decl *FromTU = getTuDecl(
+  "#define NONAME_SIZEOF(type) sizeof(struct{type *dummy;}) \n"
+  "int declToImport(){ return NONAME_SIZEOF(int); }",
+  Lang_C, "input.c");
+  auto *From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  ASSERT_TRUE(From);
+  auto *To = Import(From, Lang_C);
+  ASSERT_TRUE(To);
+  EXPECT_TRUE(MatchVerifier().match(
+  To, functionDecl(hasName("declToImport"),
+   hasDescendant(unaryExprOrTypeTraitExpr();
+}
+
+TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncParamsFromMacro) {
+  // This construct is not supported by ASTImporter.
+  Decl *FromTU = getTuDecl(
+  "#define PAIR_STRUCT(type) struct data_t{type a;type b;} \n"
+  "int declToImport(PAIR_STRUCT(int) ***d){ return 0; }",
+  Lang_C, "input.c");
+  auto *From = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
   ASSERT_TRUE(From);
-  auto To = Import(From, Lang_C);
+  auto *To = Import(From, Lang_C);
   EXPECT_EQ(To, nullptr);
 }
 


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1147,15 +1147,21 @@
   FunctionDecl *FunDecl;
   if (isa(D) && (FunDecl = dyn_cast(OrigDC)) &&
   FunDecl->hasBody()) {
-SourceRange RecR = D->getSourceRange();
-SourceRange BodyR = FunDecl->getBody()->getSourceRange();
-// If RecordDecl is not in Body (it is a param), we bail out.
-if (RecR.isValid() && BodyR.isValid() &&
-(RecR.getBegin() < BodyR.getBegin() ||
- BodyR.getEnd() < RecR.getEnd())) {
-  Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
-  << D->getDeclKindName();
-  return true;
+auto getLeafPointeeType = [](const Type *T) {
+  while (T->isPointerType() || T->isArrayType()) {
+T = T->getPoin

[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-08-06 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Ping.

Manuel, I still don't see how could we apply `match(anyOf(node), 
hasDescendant(node))` to the problem of general subtree traversal. 
(I'd like to have support for not just `decl()` but other kind of nodes too.)
Could you please advise how to move on? Also, could you please describe your 
specific technical arguments against this patch?


Repository:
  rC Clang

https://reviews.llvm.org/D49840



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


[PATCH] D50428: Add support for importing imaginary literals

2018-08-08 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a_sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Repository:
  rC Clang

https://reviews.llvm.org/D50428

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -553,6 +553,14 @@
   floatLiteral(equals(1.0e-5f), hasType(asString("float"));
 }
 
+TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
+  MatchVerifier Verifier;
+  testImport(
+  "void declToImport() { (void)1.0i; }",
+  Lang_CXX14, "", Lang_CXX14, Verifier,
+  functionDecl(hasDescendant(imaginaryLiteral(;
+}
+
 TEST_P(ImportExpr, ImportCompoundLiteralExpr) {
   MatchVerifier Verifier;
   testImport(
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -710,6 +710,7 @@
 const internal::VariadicDynCastAllOfMatcher
 integerLiteral;
 const internal::VariadicDynCastAllOfMatcher 
floatLiteral;
+const internal::VariadicDynCastAllOfMatcher 
imaginaryLiteral;
 const internal::VariadicDynCastAllOfMatcher
 userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -412,6 +412,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacterLiteral(CharacterLiteral *E);
 Expr *VisitStringLiteral(StringLiteral *E);
 Expr *VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
@@ -5594,6 +5595,7 @@
 Importer.Import(E->getLocation()));
 }
 
+
 Expr *ASTNodeImporter::VisitFloatingLiteral(FloatingLiteral *E) {
   QualType T = Importer.Import(E->getType());
   if (T.isNull())
@@ -5604,6 +5606,19 @@
 Importer.Import(E->getLocation()));
 }
 
+Expr *ASTNodeImporter::VisitImaginaryLiteral(ImaginaryLiteral *E) {
+  QualType T = Importer.Import(E->getType());
+  if (T.isNull())
+return nullptr;
+
+  Expr *SubE = Importer.Import(E->getSubExpr());
+  if (!SubE)
+return nullptr;
+
+  return new (Importer.getToContext())
+  ImaginaryLiteral(SubE, T);
+}
+
 Expr *ASTNodeImporter::VisitCharacterLiteral(CharacterLiteral *E) {
   QualType T = Importer.Import(E->getType());
   if (T.isNull())
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1975,6 +1975,11 @@
 extern const internal::VariadicDynCastAllOfMatcher
 floatLiteral;
 
+/// Matches imaginary literals, which are based on integer and floating
+/// point literals e.g.: 1i, 1.0i
+extern const internal::VariadicDynCastAllOfMatcher
+imaginaryLiteral;
+
 /// Matches user defined literal operator call.
 ///
 /// Example match: "foo"_suffix


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -553,6 +553,14 @@
   floatLiteral(equals(1.0e-5f), hasType(asString("float"));
 }
 
+TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
+  MatchVerifier Verifier;
+  testImport(
+  "void declToImport() { (void)1.0i; }",
+  Lang_CXX14, "", Lang_CXX14, Verifier,
+  functionDecl(hasDescendant(imaginaryLiteral(;
+}
+
 TEST_P(ImportExpr, ImportCompoundLiteralExpr) {
   MatchVerifier Verifier;
   testImport(
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -710,6 +710,7 @@
 const internal::VariadicDynCastAllOfMatcher
 integerLiteral;
 const internal::VariadicDynCastAllOfMatcher floatLiteral;
+const internal::VariadicDynCastAllOfMatcher imaginaryLiteral;
 const internal::VariadicDynCastAllOfMatcher
 userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -412,6 +412,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacter

[PATCH] D50428: Add support for importing imaginary literals

2018-08-08 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 159662.
martong added a comment.

Remove superflous newline


Repository:
  rC Clang

https://reviews.llvm.org/D50428

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -553,6 +553,14 @@
   floatLiteral(equals(1.0e-5f), hasType(asString("float"));
 }
 
+TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
+  MatchVerifier Verifier;
+  testImport(
+  "void declToImport() { (void)1.0i; }",
+  Lang_CXX14, "", Lang_CXX14, Verifier,
+  functionDecl(hasDescendant(imaginaryLiteral(;
+}
+
 TEST_P(ImportExpr, ImportCompoundLiteralExpr) {
   MatchVerifier Verifier;
   testImport(
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -710,6 +710,7 @@
 const internal::VariadicDynCastAllOfMatcher
 integerLiteral;
 const internal::VariadicDynCastAllOfMatcher 
floatLiteral;
+const internal::VariadicDynCastAllOfMatcher 
imaginaryLiteral;
 const internal::VariadicDynCastAllOfMatcher
 userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -412,6 +412,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacterLiteral(CharacterLiteral *E);
 Expr *VisitStringLiteral(StringLiteral *E);
 Expr *VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
@@ -5604,6 +5605,19 @@
 Importer.Import(E->getLocation()));
 }
 
+Expr *ASTNodeImporter::VisitImaginaryLiteral(ImaginaryLiteral *E) {
+  QualType T = Importer.Import(E->getType());
+  if (T.isNull())
+return nullptr;
+
+  Expr *SubE = Importer.Import(E->getSubExpr());
+  if (!SubE)
+return nullptr;
+
+  return new (Importer.getToContext())
+  ImaginaryLiteral(SubE, T);
+}
+
 Expr *ASTNodeImporter::VisitCharacterLiteral(CharacterLiteral *E) {
   QualType T = Importer.Import(E->getType());
   if (T.isNull())
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -1975,6 +1975,11 @@
 extern const internal::VariadicDynCastAllOfMatcher
 floatLiteral;
 
+/// Matches imaginary literals, which are based on integer and floating
+/// point literals e.g.: 1i, 1.0i
+extern const internal::VariadicDynCastAllOfMatcher
+imaginaryLiteral;
+
 /// Matches user defined literal operator call.
 ///
 /// Example match: "foo"_suffix


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -553,6 +553,14 @@
   floatLiteral(equals(1.0e-5f), hasType(asString("float"));
 }
 
+TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
+  MatchVerifier Verifier;
+  testImport(
+  "void declToImport() { (void)1.0i; }",
+  Lang_CXX14, "", Lang_CXX14, Verifier,
+  functionDecl(hasDescendant(imaginaryLiteral(;
+}
+
 TEST_P(ImportExpr, ImportCompoundLiteralExpr) {
   MatchVerifier Verifier;
   testImport(
Index: lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- lib/ASTMatchers/ASTMatchersInternal.cpp
+++ lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -710,6 +710,7 @@
 const internal::VariadicDynCastAllOfMatcher
 integerLiteral;
 const internal::VariadicDynCastAllOfMatcher floatLiteral;
+const internal::VariadicDynCastAllOfMatcher imaginaryLiteral;
 const internal::VariadicDynCastAllOfMatcher
 userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -412,6 +412,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacterLiteral(CharacterLiteral *E);
 Expr *VisitStringLiteral(StringLiteral *E);
 Expr *VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
@@ -5604,6 +5605,19 @@
 Importer.Import(E->getLocation()));
 }
 
+Expr *ASTNodeImporter::VisitImaginaryL

[PATCH] D50444: Fix structural inequivalency of forward EnumDecl

2018-08-08 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a_sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Currently we consider one forward declared RecordDecl and another with a
definition equal. We have to do the same in case of enums.


Repository:
  rC Clang

https://reviews.llvm.org/D50444

Files:
  lib/AST/ASTStructuralEquivalence.cpp
  unittests/AST/StructuralEquivalenceTest.cpp


Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -618,6 +618,31 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest,
+FwdDeclRecordShouldBeEqualWithFwdDeclRecord) {
+  auto t = makeNamedDecls("class foo;", "class foo;", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   FwdDeclRecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t =
+  makeNamedDecls("class foo;", "class foo { int A; };", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t = makeNamedDecls("class foo { int A; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest, RecordsWithDifferentBody) {
+  auto t = makeNamedDecls("class foo { int B; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
 
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
@@ -627,5 +652,33 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceEnumTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceEnumTest, FwdDeclEnumShouldBeEqualWithFwdDeclEnum) 
{
+  auto t = makeNamedDecls("enum class foo;", "enum class foo;", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest,
+   FwdDeclEnumShouldBeEqualWithEnumWhichHasDefinition) {
+  auto t =
+  makeNamedDecls("enum class foo;", "enum class foo { A };", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest,
+   EnumShouldBeEqualWithEnumWhichHasDefinition) {
+  auto t = makeNamedDecls("enum class foo { A };", "enum class foo { A };",
+  Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest, EnumsWithDifferentBody) {
+  auto t = makeNamedDecls("enum class foo { B };", "enum class foo { A };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/ASTStructuralEquivalence.cpp
===
--- lib/AST/ASTStructuralEquivalence.cpp
+++ lib/AST/ASTStructuralEquivalence.cpp
@@ -1178,6 +1178,14 @@
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  EnumDecl *D1, EnumDecl *D2) {
+
+  // Compare the definitions of these two eunums. If either or both are
+  // incomplete (i.e. forward declared), we assume that they are equivalent.
+  D1 = D1->getDefinition();
+  D2 = D2->getDefinition();
+  if (!D1 || !D2)
+return true;
+
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
 EC2End = D2->enumerator_end();
   for (EnumDecl::enumerator_iterator EC1 = D1->enumerator_begin(),


Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -618,6 +618,31 @@
   EXPECT_FALSE(testStructuralMatch(R0, R1));
 }
 
+TEST_F(StructuralEquivalenceRecordTest,
+FwdDeclRecordShouldBeEqualWithFwdDeclRecord) {
+  auto t = makeNamedDecls("class foo;", "class foo;", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   FwdDeclRecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t =
+  makeNamedDecls("class foo;", "class foo { int A; };", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t = makeNamedDecls("class foo { int A; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest, RecordsWithDifferentBody) {
+  auto t = makeNamedDecls("class foo { int B; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
 
 TEST_F(StructuralEquivalenceTest, Co

[PATCH] D50451: Fix import of class templates partial specialization

2018-08-08 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a_sidorin, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Currently there are several issues with the import of class template
specializations.  (1) Different TUs may have class template specializations
with the same template arguments, but with different set of instantiated
MethodDecls and FieldDecls.  In this patch we provide a fix to merge these
methods and fields.  (2) Currently, we search the partial template
specializations in the set of simple specializations and we add partial
specializations as simple specializations. This is bad, this patch fixes it.


Repository:
  rC Clang

https://reviews.llvm.org/D50451

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2641,6 +2641,216 @@
   R1, recordDecl(has(fieldDecl(hasName("next"));
 }
 
+TEST_P(ASTImporterTestBase, MergeFieldDeclsOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  int a{0}; // FieldDecl with InitListExpr
+  X(char) : a(3) {} // (1)
+  X(int) {} // (2)
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  // ClassTemplateSpec with ctor (1): FieldDecl without InitlistExpr
+  X xc('c');
+  }
+  )", Lang_CXX11);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl without InitlistExpr:
+  auto *ToField = *ToSpec->field_begin();
+  ASSERT_TRUE(ToField);
+  ASSERT_FALSE(ToField->getInClassInitializer());
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  // ClassTemplateSpec with ctor (2): FieldDecl WITH InitlistExpr
+  X xc(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl with InitlistExpr:
+  auto *FromField = *FromSpec->field_begin();
+  ASSERT_TRUE(FromField);
+  ASSERT_TRUE(FromField->getInClassInitializer());
+
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  // After the import, the FieldDecl has to be merged, thus it should have the
+  // InitListExpr.
+  EXPECT_TRUE(ToField->getInClassInitializer());
+}
+
+TEST_P(ASTImporterTestBase, MergeFunctionOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x;
+  x.f();
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x;
+  x.g();
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto FunPattern = functionDecl(hasName("g"),
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromFun =
+  FirstDeclMatcher().match(FromTU, FunPattern);
+  auto *ToFun =
+  FirstDeclMatcher().match(ToTU, FunPattern);
+  ASSERT_TRUE(FromFun->hasBody());
+  ASSERT_FALSE(ToFun->hasBody());
+   auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+   ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  EXPECT_TRUE(ToFun->hasBody());
+}
+
+TEST_P(ASTImporterTestBase, MergeCtorOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  X(char) {}
+  X(int) {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x('c');
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // Match the void(int) ctor.
+  auto CtorPattern =
+  cxxConstructorDecl(hasParameter(0, varDecl(hasType(asString("int",
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromCtor =
+  FirstDeclMatcher().match(FromTU, CtorPattern);
+  auto *ToCtor =
+  FirstDeclMatcher().match(ToTU, CtorPattern);
+  ASSERT_TRUE(FromCtor->hasBody());
+  ASSERT_FALSE(ToCtor->hasBody());
+   auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(

[PATCH] D50428: [ASTImporter] Add support for importing imaginary literals

2018-08-09 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339334: Add support for importing imaginary literals 
(authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50428?vs=159662&id=159902#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50428

Files:
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp


Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -1975,6 +1975,11 @@
 extern const internal::VariadicDynCastAllOfMatcher
 floatLiteral;
 
+/// Matches imaginary literals, which are based on integer and floating
+/// point literals e.g.: 1i, 1.0i
+extern const internal::VariadicDynCastAllOfMatcher
+imaginaryLiteral;
+
 /// Matches user defined literal operator call.
 ///
 /// Example match: "foo"_suffix
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -434,6 +434,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacterLiteral(CharacterLiteral *E);
 Expr *VisitStringLiteral(StringLiteral *E);
 Expr *VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
@@ -5613,6 +5614,18 @@
 Importer.Import(E->getLocation()));
 }
 
+Expr *ASTNodeImporter::VisitImaginaryLiteral(ImaginaryLiteral *E) {
+  QualType T = Importer.Import(E->getType());
+  if (T.isNull())
+return nullptr;
+
+  Expr *SubE = Importer.Import(E->getSubExpr());
+  if (!SubE)
+return nullptr;
+
+  return new (Importer.getToContext()) ImaginaryLiteral(SubE, T);
+}
+
 Expr *ASTNodeImporter::VisitCharacterLiteral(CharacterLiteral *E) {
   QualType T = Importer.Import(E->getType());
   if (T.isNull())
Index: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -710,6 +710,7 @@
 const internal::VariadicDynCastAllOfMatcher
 integerLiteral;
 const internal::VariadicDynCastAllOfMatcher 
floatLiteral;
+const internal::VariadicDynCastAllOfMatcher 
imaginaryLiteral;
 const internal::VariadicDynCastAllOfMatcher
 userDefinedLiteral;
 const internal::VariadicDynCastAllOfMatcher
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -553,6 +553,14 @@
   floatLiteral(equals(1.0e-5f), hasType(asString("float"));
 }
 
+TEST_P(ImportExpr, ImportImaginaryLiteralExpr) {
+  MatchVerifier Verifier;
+  testImport(
+  "void declToImport() { (void)1.0i; }",
+  Lang_CXX14, "", Lang_CXX14, Verifier,
+  functionDecl(hasDescendant(imaginaryLiteral(;
+}
+
 TEST_P(ImportExpr, ImportCompoundLiteralExpr) {
   MatchVerifier Verifier;
   testImport(


Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -1975,6 +1975,11 @@
 extern const internal::VariadicDynCastAllOfMatcher
 floatLiteral;
 
+/// Matches imaginary literals, which are based on integer and floating
+/// point literals e.g.: 1i, 1.0i
+extern const internal::VariadicDynCastAllOfMatcher
+imaginaryLiteral;
+
 /// Matches user defined literal operator call.
 ///
 /// Example match: "foo"_suffix
Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -434,6 +434,7 @@
 Expr *VisitCXXNullPtrLiteralExpr(CXXNullPtrLiteralExpr *E);
 Expr *VisitIntegerLiteral(IntegerLiteral *E);
 Expr *VisitFloatingLiteral(FloatingLiteral *E);
+Expr *VisitImaginaryLiteral(ImaginaryLiteral *E);
 Expr *VisitCharacterLiteral(CharacterLiteral *E);
 Expr *VisitStringLiteral(StringLiteral *E);
 Expr *VisitCompoundLiteralExpr(CompoundLiteralExpr *E);
@@ -5613,6 +5614,18 @@
 Importer.Import(E->getLocation()));
 }
 
+Expr *ASTNodeImporter::VisitImaginaryLiteral(ImaginaryLiteral *E) {
+  QualType T = Importer.Import(E->getType());
+  if (T.isNull())
+return nullptr;
+
+  Expr *SubE = Importer.Import(E

[PATCH] D50444: [ASTImporter] Fix structural inequivalency of forward EnumDecl

2018-08-09 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339336: Fix structural inequivalency of forward EnumDecl 
(authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50444?vs=159697&id=159905#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50444

Files:
  cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
  cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp


Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -1178,6 +1178,14 @@
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  EnumDecl *D1, EnumDecl *D2) {
+
+  // Compare the definitions of these two enums. If either or both are
+  // incomplete (i.e. forward declared), we assume that they are equivalent.
+  D1 = D1->getDefinition();
+  D2 = D2->getDefinition();
+  if (!D1 || !D2)
+return true;
+
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
 EC2End = D2->enumerator_end();
   for (EnumDecl::enumerator_iterator EC1 = D1->enumerator_begin(),
Index: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
===
--- cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
+++ cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
@@ -642,13 +642,67 @@
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceRecordTest,
+FwdDeclRecordShouldBeEqualWithFwdDeclRecord) {
+  auto t = makeNamedDecls("class foo;", "class foo;", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   FwdDeclRecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t =
+  makeNamedDecls("class foo;", "class foo { int A; };", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+   RecordShouldBeEqualWithRecordWhichHasDefinition) {
+  auto t = makeNamedDecls("class foo { int A; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceRecordTest, RecordsWithDifferentBody) {
+  auto t = makeNamedDecls("class foo { int B; };", "class foo { int A; };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
   "struct A{ }; struct B{ }; void foo(A a, A b);",
   "struct A{ }; struct B{ }; void foo(A a, B b);",
   Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+struct StructuralEquivalenceEnumTest : StructuralEquivalenceTest {};
+
+TEST_F(StructuralEquivalenceEnumTest, FwdDeclEnumShouldBeEqualWithFwdDeclEnum) 
{
+  auto t = makeNamedDecls("enum class foo;", "enum class foo;", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest,
+   FwdDeclEnumShouldBeEqualWithEnumWhichHasDefinition) {
+  auto t =
+  makeNamedDecls("enum class foo;", "enum class foo { A };", Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest,
+   EnumShouldBeEqualWithEnumWhichHasDefinition) {
+  auto t = makeNamedDecls("enum class foo { A };", "enum class foo { A };",
+  Lang_CXX11);
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceEnumTest, EnumsWithDifferentBody) {
+  auto t = makeNamedDecls("enum class foo { B };", "enum class foo { A };",
+  Lang_CXX11);
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
+
 } // end namespace ast_matchers
 } // end namespace clang


Index: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
===
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
@@ -1178,6 +1178,14 @@
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
  EnumDecl *D1, EnumDecl *D2) {
+
+  // Compare the definitions of these two enums. If either or both are
+  // incomplete (i.e. forward declared), we assume that they are equivalent.
+  D1 = D1->getDefinition();
+  D2 = D2->getDefinition();
+  if (!D1 || !D2)
+return true;
+
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
 EC2End = D2->enumerator_end();
   for (EnumDecl::enumerator_iterator EC1 = D1->enumerator_begin(),
Index: cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp
===
--- cfe/trunk/unittests/AS

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong marked 5 inline comments as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2872
 Importer.MapImported(D, FoundField);
+// In case of a FieldDecl of a ClassTemplateSpecializationDecl, the
+// initializer of a FieldDecl might not had been instantiated in the

a_sidorin wrote:
> Honestly speaking, I wonder about this behaviour because it doesn't look 
> similar to instantiation of only methods that are used. Is it a common rule?
Yes, this is a common rule. The instantiation of an initializer is similar to 
the instantiation of default arguments in a sense that both are instantated 
only if they are being used.

To be more precise, quoting from Vandevoorde - C++ Templates The Complete Guide 
/ 14.2.2 Instantiated Components:

... Default functioncallarguments   are considered  
separately  wheninstantiating
templates.  Specifically,   theyare not instantiatedunless  
there   is  a   callto  that
function(or member  function)   thatactuallymakes   
use of  the default argument.
If, on  the other   hand,   the functionis  called  
withexplicitarguments   thatoverride
the default,thenthe default arguments   are not 
instantiated.
Similarly,  exception   specifications  and **default   member  
initializers**  are not
instantiatedunless  theyare needed.





Comment at: lib/AST/ASTImporter.cpp:4550
+  // in the "From" context, but not in the "To" context.
+  for (auto *FromField : D->fields())
+Importer.Import(FromField);

a_sidorin wrote:
> Importing additional fields can change the layout of the specialization. For 
> CSA, this usually results in strange assertions hard to debug. Could you 
> please share the results of testing of this change?
> This change also doesn't seem to have related tests in this patch.
TLDR; We will not create additional fields.

By the time when we import the field, we already know that the existing 
specialization is structurally equivalent with the new one. 
Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, the 
structural equivalence check ensures that they have the exact same fields.
When we import the field of the new spec and if there is an existing FieldDecl 
in the "To" context, then no new FieldDecl will be created (this is handled in 
`VisitFieldDecl` by first doing a lookup of existing field with the same name 
and type).
This patch extends `VisitFieldDecl` in a way that we add new initializer 
expressions to the existing FieldDecl, if it didn't have and in the "From" 
context it has.

For the record, I  added a new test to make sure that a new FieldDecl will not 
be created during the merge.



Comment at: lib/AST/ASTImporter.cpp:4551
+  for (auto *FromField : D->fields())
+Importer.Import(FromField);
+

a_sidorin wrote:
> The result of import is unchecked here and below. Is it intentional?
Yes, that is intentional. We plan to refactor all ASTImporter functions to 
provide a proper error handling mechanism, and in that change we would like to 
enforce the check of all import functions.
Unfortunately, currently we already have many places where we do not check the 
return value of import.


Repository:
  rC Clang

https://reviews.llvm.org/D50451



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


[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong marked 3 inline comments as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4551
+  for (auto *FromField : D->fields())
+Importer.Import(FromField);
+

martong wrote:
> a_sidorin wrote:
> > The result of import is unchecked here and below. Is it intentional?
> Yes, that is intentional. We plan to refactor all ASTImporter functions to 
> provide a proper error handling mechanism, and in that change we would like 
> to enforce the check of all import functions.
> Unfortunately, currently we already have many places where we do not check 
> the return value of import.
Actually, having proper error handling is just one thing, the other more 
important reason why we don't check the result of the import here, because we 
don't want to stop merging other fields/functions if one merge failed. If one 
merge failed, other merge operations still could be successful.


Repository:
  rC Clang

https://reviews.llvm.org/D50451



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


[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 160552.
martong added a comment.

- Add new test
- Fix indentation


Repository:
  rC Clang

https://reviews.llvm.org/D50451

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2683,6 +2683,263 @@
   EXPECT_EQ(FromIndex, 3u);
 }
 
+TEST_P(ASTImporterTestBase, MergeFieldDeclsOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  int a{0}; // FieldDecl with InitListExpr
+  X(char) : a(3) {} // (1)
+  X(int) {} // (2)
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  // ClassTemplateSpec with ctor (1): FieldDecl without InitlistExpr
+  X xc('c');
+  }
+  )", Lang_CXX11);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl without InitlistExpr:
+  auto *ToField = *ToSpec->field_begin();
+  ASSERT_TRUE(ToField);
+  ASSERT_FALSE(ToField->getInClassInitializer());
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  // ClassTemplateSpec with ctor (2): FieldDecl WITH InitlistExpr
+  X xc(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl with InitlistExpr:
+  auto *FromField = *FromSpec->field_begin();
+  ASSERT_TRUE(FromField);
+  ASSERT_TRUE(FromField->getInClassInitializer());
+
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  // After the import, the FieldDecl has to be merged, thus it should have the
+  // InitListExpr.
+  EXPECT_TRUE(ToField->getInClassInitializer());
+}
+
+TEST_P(ASTImporterTestBase, MergeFunctionOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x;
+  x.f();
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x;
+  x.g();
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto FunPattern = functionDecl(hasName("g"),
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromFun =
+  FirstDeclMatcher().match(FromTU, FunPattern);
+  auto *ToFun =
+  FirstDeclMatcher().match(ToTU, FunPattern);
+  ASSERT_TRUE(FromFun->hasBody());
+  ASSERT_FALSE(ToFun->hasBody());
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  EXPECT_TRUE(ToFun->hasBody());
+}
+
+TEST_P(ASTImporterTestBase,
+   ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {};
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int a;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int b;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+
+  // We expect one (ODR) warning during the import.
+  EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+
+  // 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()));
+}
+
+TEST_P(ASTImporterTestBase, MergeCtorOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  X(char) {}
+  X(int) {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4550
+  // in the "From" context, but not in the "To" context.
+  for (auto *FromField : D->fields())
+Importer.Import(FromField);

martong wrote:
> a_sidorin wrote:
> > Importing additional fields can change the layout of the specialization. 
> > For CSA, this usually results in strange assertions hard to debug. Could 
> > you please share the results of testing of this change?
> > This change also doesn't seem to have related tests in this patch.
> TLDR; We will not create additional fields.
> 
> By the time when we import the field, we already know that the existing 
> specialization is structurally equivalent with the new one. 
> Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, the 
> structural equivalence check ensures that they have the exact same fields.
> When we import the field of the new spec and if there is an existing 
> FieldDecl in the "To" context, then no new FieldDecl will be created (this is 
> handled in `VisitFieldDecl` by first doing a lookup of existing field with 
> the same name and type).
> This patch extends `VisitFieldDecl` in a way that we add new initializer 
> expressions to the existing FieldDecl, if it didn't have and in the "From" 
> context it has.
> 
> For the record, I  added a new test to make sure that a new FieldDecl will 
> not be created during the merge.
This is the new test: 
`ODRViolationOfClassTemplateSpecializationsShouldBeReported`. It checks that it 
is not possible to add new fields to a specialization, rather an ODR violation 
is diagnosed.


Repository:
  rC Clang

https://reviews.llvm.org/D50451



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


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

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:1317
+  for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));

Is it sure that `ToD` will never be a nullptr?
I think, `removeDecl` or `addDeclInternal` below may crash if we call it with a 
nullptr.
Also in the assert, `ToD->getDeclContext()` seems achy if `ToD` is a nullptr.


Repository:
  rC Clang

https://reviews.llvm.org/D44100



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


[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Hi Aleksei,

Thank you for this patch.
With Balazs, we are working on something similar, but with a different, fine 
grained error value mechanism. Unfortunately we were not aware of that you have 
been working on error handling, and we didn't say that we are working on error 
handling recently, I am terribly sorry about this.

From the CSA perspective, we realized that there may be several different error 
cases which has to be handled differently in `CrossTranslationUnit.cpp`.
For example, there are unsupported constructs which we do not support to import 
(like a struct definition as a parameter of a function).
Another example is when there is a name conflict between the decl in the "From" 
context and the decl in the "To" context, this usually means an ODR error.
We have to handle these errors in a different way after we imported one 
function during CTU analysis.
The fact that there may be more than one kind of errors yields for the use of 
the designated LLVM types: `Error` and `Expected`. A simple `Optional` is 
probably not generic enough.

I find the `importNonNull` and generally the new family of `import` functions 
useful, but I am not sure how they could cooperate with `Expected`. 
Especially, I have some concerns with output parameters.
If we have an Expected as a result type, then there is no way to acquire the 
T if there was an error. However, when we have output parameters, then even if 
there was an error some output params could have been set ... and those can be 
reused even after the return. If error handling is not proper on the callee 
site then we may continue with stale values, which is not possible if we use 
Expected as a return value.

Do you think we can hold back this patch for a few days until Balazs prepares 
the `Expected` based version? Then I think we could compare the patches and 
we could merge the best from the two of them.


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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


[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> Do you think we can hold back this patch for a few days until Balazs prepares 
> the Expected based version? Then I think we could compare the patches and 
> we could merge the best from the two of them.

I mean, once Balazs is also ready, then we can create a combined patch which 
holds the good things from both of yours and his patches.


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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


[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

2018-08-14 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Roughly, this is the direction where we are heading with `Expected`:
https://github.com/Ericsson/clang/pull/457/commits/783b7f5cce9de589a5c3c3ae983398cf499077ec

(If you are interested, here is our whole thought process:
https://github.com/Ericsson/clang/pull/457)


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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


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

2018-08-15 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/AST/ASTImporter.cpp:1317
+  for (auto *D : FromRD->decls()) {
+Decl *ToD = Importer.GetAlreadyImportedOrNull(D);
+assert(ToRD == ToD->getDeclContext() && ToRD->containsDecl(ToD));

a_sidorin wrote:
> martong wrote:
> > Is it sure that `ToD` will never be a nullptr?
> > I think, `removeDecl` or `addDeclInternal` below may crash if we call it 
> > with a nullptr.
> > Also in the assert, `ToD->getDeclContext()` seems achy if `ToD` is a 
> > nullptr.
> We have an early return if such import failed before (line 1300).
Okay, that line did not catch my attention. Could you please provide a comment 
which describes that at this point we expect ToD to be non-null, or even 
better, we could have an explicit `assert(ToD) `. 


Repository:
  rC Clang

https://reviews.llvm.org/D44100



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


[PATCH] D50792: [ASTImporter] Add test for member pointer types.

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50792



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


[PATCH] D50793: [ASTImporter] Add test for importing CompoundAssignOperators

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Looks good.


Repository:
  rC Clang

https://reviews.llvm.org/D50793



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


[PATCH] D50810: [ASTImporter] Add test for DoStmt

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50810



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


[PATCH] D50811: [ASTImporter] Add test for WhileStmt

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50811



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


[PATCH] D50813: [ASTImporter] Add test for IndirectGotoStmt

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50813



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


[PATCH] D50672: [ASTImporter] Change the return result of Decl import to Optional

2018-08-16 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> Yes, I was thinking about it too. The reason why I chose Optional was that 
> ASTImporter clients don't use the error kind. If you have any plans on 
> changing this, Expected is a preferred approach.

Yes, we plan to submit a patch to LLDB too which would apply the use of 
Expected.

> If I understand you correctly, importNonNull and importNullable should work 
> with Expected pretty well.

Yes, that's right.

> Changing import*Into return result to Error can partially help in avoiding 
> usage of an unchecked result. These functions already guarantee that their 
> parameters are changed only if the internal import was successful.

Yes I agree, that `Error` can help, and I also agree that this help is just 
partial.
If we change `importNonNullInto` to has an `Error` return value

  template 
  LLVM_NODISCARD Error importNonNullInto(DeclTy *&ToD, Decl *FromD) {
if (auto Res = Importer.Import(FromD)) {
  ToD = cast(*Res);
  return make_error();
}
return Error::success();
  }

then on the call site, on the branch where whe handle the error we may make 
mistakes like this:

  CXXRecordDecl *ToTemplated = nullptr;
  if (Err = importNonNullInto(ToTemplated, FromTemplated)) {
foo(ToTemplated->hasDefinition()); // Undefined Behaviour!
return Err;
  }

If we do not use output parameters, only `Expected`, then this could not 
happen.
`Expected.get()` aborts if there was an error.


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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


[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-21 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 161698.
martong marked an inline comment as done.
martong added a comment.

- Change comments


Repository:
  rC Clang

https://reviews.llvm.org/D50451

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2683,6 +2683,263 @@
   EXPECT_EQ(FromIndex, 3u);
 }
 
+TEST_P(ASTImporterTestBase, MergeFieldDeclsOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  int a{0}; // FieldDecl with InitListExpr
+  X(char) : a(3) {} // (1)
+  X(int) {} // (2)
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  // ClassTemplateSpec with ctor (1): FieldDecl without InitlistExpr
+  X xc('c');
+  }
+  )", Lang_CXX11);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl without InitlistExpr:
+  auto *ToField = *ToSpec->field_begin();
+  ASSERT_TRUE(ToField);
+  ASSERT_FALSE(ToField->getInClassInitializer());
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  // ClassTemplateSpec with ctor (2): FieldDecl WITH InitlistExpr
+  X xc(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl with InitlistExpr:
+  auto *FromField = *FromSpec->field_begin();
+  ASSERT_TRUE(FromField);
+  ASSERT_TRUE(FromField->getInClassInitializer());
+
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  // After the import, the FieldDecl has to be merged, thus it should have the
+  // InitListExpr.
+  EXPECT_TRUE(ToField->getInClassInitializer());
+}
+
+TEST_P(ASTImporterTestBase, MergeFunctionOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x;
+  x.f();
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x;
+  x.g();
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto FunPattern = functionDecl(hasName("g"),
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromFun =
+  FirstDeclMatcher().match(FromTU, FunPattern);
+  auto *ToFun =
+  FirstDeclMatcher().match(ToTU, FunPattern);
+  ASSERT_TRUE(FromFun->hasBody());
+  ASSERT_FALSE(ToFun->hasBody());
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  EXPECT_TRUE(ToFun->hasBody());
+}
+
+TEST_P(ASTImporterTestBase,
+   ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {};
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int a;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int b;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+
+  // We expect one (ODR) warning during the import.
+  EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+
+  // 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()));
+}
+
+TEST_P(ASTImporterTestBase, MergeCtorOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  X(char) {}
+  X(int) {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl

[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-21 Thread Gabor Marton via Phabricator via cfe-commits
martong marked an inline comment as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4550
+  // in the "From" context, but not in the "To" context.
+  for (auto *FromField : D->fields())
+Importer.Import(FromField);

a_sidorin wrote:
> martong wrote:
> > martong wrote:
> > > a_sidorin wrote:
> > > > Importing additional fields can change the layout of the 
> > > > specialization. For CSA, this usually results in strange assertions 
> > > > hard to debug. Could you please share the results of testing of this 
> > > > change?
> > > > This change also doesn't seem to have related tests in this patch.
> > > TLDR; We will not create additional fields.
> > > 
> > > By the time when we import the field, we already know that the existing 
> > > specialization is structurally equivalent with the new one. 
> > > Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, 
> > > the structural equivalence check ensures that they have the exact same 
> > > fields.
> > > When we import the field of the new spec and if there is an existing 
> > > FieldDecl in the "To" context, then no new FieldDecl will be created 
> > > (this is handled in `VisitFieldDecl` by first doing a lookup of existing 
> > > field with the same name and type).
> > > This patch extends `VisitFieldDecl` in a way that we add new initializer 
> > > expressions to the existing FieldDecl, if it didn't have and in the 
> > > "From" context it has.
> > > 
> > > For the record, I  added a new test to make sure that a new FieldDecl 
> > > will not be created during the merge.
> > This is the new test: 
> > `ODRViolationOfClassTemplateSpecializationsShouldBeReported`. It checks 
> > that it is not possible to add new fields to a specialization, rather an 
> > ODR violation is diagnosed.
> Thank you for the explanation. However, I find the comment very misleading. 
> It tells: 
> ```
>   // Check and merge those fields which have been instantiated
>   // in the "From" context, but not in the "To" context.
> ```
> Would it be correct to change it to "Import field initializers that are still 
> not instantiated", or do I still misunderstand something?
Yes, I agree that this comment is not precise and could be misleading, thank 
you for pointing this out. I changed this comment and others too.


Repository:
  rC Clang

https://reviews.llvm.org/D50451



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


[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-21 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 161699.
martong added a comment.

- Add one more TODO to import instantiated exception specifications


Repository:
  rC Clang

https://reviews.llvm.org/D50451

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -2683,6 +2683,263 @@
   EXPECT_EQ(FromIndex, 3u);
 }
 
+TEST_P(ASTImporterTestBase, MergeFieldDeclsOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  int a{0}; // FieldDecl with InitListExpr
+  X(char) : a(3) {} // (1)
+  X(int) {} // (2)
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  // ClassTemplateSpec with ctor (1): FieldDecl without InitlistExpr
+  X xc('c');
+  }
+  )", Lang_CXX11);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl without InitlistExpr:
+  auto *ToField = *ToSpec->field_begin();
+  ASSERT_TRUE(ToField);
+  ASSERT_FALSE(ToField->getInClassInitializer());
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  // ClassTemplateSpec with ctor (2): FieldDecl WITH InitlistExpr
+  X xc(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl with InitlistExpr:
+  auto *FromField = *FromSpec->field_begin();
+  ASSERT_TRUE(FromField);
+  ASSERT_TRUE(FromField->getInClassInitializer());
+
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  // After the import, the FieldDecl has to be merged, thus it should have the
+  // InitListExpr.
+  EXPECT_TRUE(ToField->getInClassInitializer());
+}
+
+TEST_P(ASTImporterTestBase, MergeFunctionOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x;
+  x.f();
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x;
+  x.g();
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto FunPattern = functionDecl(hasName("g"),
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromFun =
+  FirstDeclMatcher().match(FromTU, FunPattern);
+  auto *ToFun =
+  FirstDeclMatcher().match(ToTU, FunPattern);
+  ASSERT_TRUE(FromFun->hasBody());
+  ASSERT_FALSE(ToFun->hasBody());
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  EXPECT_TRUE(ToFun->hasBody());
+}
+
+TEST_P(ASTImporterTestBase,
+   ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {};
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int a;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int b;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+
+  // We expect one (ODR) warning during the import.
+  EXPECT_EQ(1u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+
+  // 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()));
+}
+
+TEST_P(ASTImporterTestBase, MergeCtorOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  X(char) {}
+  X(int) {}
+  };
+  )";
+  Decl *ToTU = get

[PATCH] D51001: [ASTImporter] Add test for CXXForRangeStmt

2018-08-21 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D51001



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


[PATCH] D50451: [ASTImporter] Fix import of class templates partial specialization

2018-08-22 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340402: Fix import of class templates partial specialization 
(authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50451?vs=161699&id=161923#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50451

Files:
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -2955,19 +2955,276 @@
   auto *FromD = FirstDeclMatcher().match(
   FromTu, classTemplateDecl(hasName("declToImport")));
   auto *ToD = Import(FromD, Lang_CXX);
-  
+
   auto Pattern = classTemplateDecl(
   has(cxxRecordDecl(has(friendDecl(has(classTemplateDecl()));
   ASSERT_TRUE(MatchVerifier{}.match(FromD, Pattern));
   EXPECT_TRUE(MatchVerifier{}.match(ToD, Pattern));
-  
+
   auto *Class =
   FirstDeclMatcher().match(ToD, classTemplateDecl());
   auto *Friend = FirstDeclMatcher().match(ToD, friendDecl());
   EXPECT_NE(Friend->getFriendDecl(), Class);
   EXPECT_EQ(Friend->getFriendDecl()->getPreviousDecl(), Class);
 }
 
+TEST_P(ASTImporterTestBase, MergeFieldDeclsOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+  int a{0}; // FieldDecl with InitListExpr
+  X(char) : a(3) {} // (1)
+  X(int) {} // (2)
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  // ClassTemplateSpec with ctor (1): FieldDecl without InitlistExpr
+  X xc('c');
+  }
+  )", Lang_CXX11);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl without InitlistExpr:
+  auto *ToField = *ToSpec->field_begin();
+  ASSERT_TRUE(ToField);
+  ASSERT_FALSE(ToField->getInClassInitializer());
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  // ClassTemplateSpec with ctor (2): FieldDecl WITH InitlistExpr
+  X xc(1);
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  // FieldDecl with InitlistExpr:
+  auto *FromField = *FromSpec->field_begin();
+  ASSERT_TRUE(FromField);
+  ASSERT_TRUE(FromField->getInClassInitializer());
+
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  // After the import, the FieldDecl has to be merged, thus it should have the
+  // InitListExpr.
+  EXPECT_TRUE(ToField->getInClassInitializer());
+}
+
+TEST_P(ASTImporterTestBase, MergeFunctionOfClassTemplateSpecialization) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+  R"(
+  void foo() {
+  X x;
+  x.f();
+  }
+  )", Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+  R"(
+  void bar() {
+  X x;
+  x.g();
+  }
+  )", Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto FunPattern = functionDecl(hasName("g"),
+ hasParent(classTemplateSpecializationDecl()));
+  auto *FromFun =
+  FirstDeclMatcher().match(FromTU, FunPattern);
+  auto *ToFun =
+  FirstDeclMatcher().match(ToTU, FunPattern);
+  ASSERT_TRUE(FromFun->hasBody());
+  ASSERT_FALSE(ToFun->hasBody());
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);
+  auto *ToSpec = FirstDeclMatcher().match(
+  ToTU, classTemplateSpecializationDecl(hasName("X")));
+  EXPECT_EQ(ImportedSpec, ToSpec);
+  EXPECT_TRUE(ToFun->hasBody());
+}
+
+TEST_P(ASTImporterTestBase,
+   ODRViolationOfClassTemplateSpecializationsShouldBeReported) {
+  std::string ClassTemplate =
+  R"(
+  template 
+  struct X {};
+  )";
+  Decl *ToTU = getToTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int a;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  Decl *FromTU = getTuDecl(ClassTemplate +
+   R"(
+  template <>
+  struct X {
+  int b;
+  };
+  void foo() {
+  X x;
+  }
+  )",
+   Lang_CXX11);
+  auto *FromSpec = FirstDeclMatcher().match(
+  FromTU, classTemplateSpecializationDecl(hasName("X")));
+  auto *ImportedSpec = Import(FromSpec, Lang_CXX11

[PATCH] D46398: [ASTImporterTest] Fix potential use-after-free

2018-05-14 Thread 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!


Repository:
  rC Clang

https://reviews.llvm.org/D46398



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


[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-14 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: xazax.hun, a.sidorin.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

`DeclContext` is essentially a list of `Decl`s and a lookup table (`LookupPtr`) 
but these are encapsulated.
E.g. `LookupPtr` is private. `DeclContext::removeDecl` has the responsibility 
to remove the given decl from the list and from the table too.
Template specializations cannot participate in normal (qualified) lookup.
Consequently no template specialization can be in the lookup table, but they 
will be in the list. When the lookup table is built or when a new `Decl` is 
added, then it is checked whether it is a template specialization and if so it 
is skipped from the lookup table.
Thus, whenever we want to remove a `Decl` from a `DeclContext` we must not 
reach the point to remove that from the lookup table (and that is what this 
patch do).

With respect to ASTImporter: At some point probably we will be reordering 
`FriendDecl`s and possibly `CXXMethodDecl`s in a `RecordDecl` at 
`importDeclContext` to be able to structurally compare two `RecordDecl`s.
When that happens we most probably want to remove all `Decls` from a 
`RecordDecl` and then we would add them in the proper order.


Repository:
  rC Clang

https://reviews.llvm.org/D46835

Files:
  lib/AST/DeclBase.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/MatchVerifier.h

Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,11 +28,12 @@
 namespace clang {
 namespace ast_matchers {
 
-enum Language { 
+enum Language {
 Lang_C,
 Lang_C89,
 Lang_CXX,
 Lang_CXX11,
+Lang_CXX14,
 Lang_OpenCL,
 Lang_OBJCXX
 };
@@ -113,6 +114,10 @@
 Args.push_back("-std=c++11");
 FileName = "input.cc";
 break;
+  case Lang_CXX14:
+Args.push_back("-std=c++14");
+FileName = "input.cc";
+break;
   case Lang_OpenCL:
 FileName = "input.cl";
 break;
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -52,6 +52,9 @@
   case Lang_CXX11:
 BasicArgs = {"-std=c++11", "-frtti"};
 break;
+  case Lang_CXX14:
+BasicArgs = {"-std=c++14", "-frtti"};
+break;
   case Lang_OpenCL:
   case Lang_OBJCXX:
 llvm_unreachable("Not implemented yet!");
@@ -1764,5 +1767,86 @@
  compoundStmt(has(callExpr(has(unresolvedMemberExpr());
 }
 
+TEST(ImportExpr, ImportClassTemplatePartialSpecialization) {
+  MatchVerifier Verifier;
+  auto Code =
+R"s(
+struct declToImport {
+  template 
+  struct X {};
+  template 
+  struct X {};
+  X x0;
+};
+)s";
+
+  testImport(Code, Lang_CXX, "", Lang_CXX, Verifier, recordDecl());
+}
+
+TEST(ImportExpr, ImportSpecializationsOfFriendClassTemplate) {
+  MatchVerifier Verifier;
+  auto Code =
+R"(
+namespace declToImport {
+
+template 
+struct F{};
+
+class X {
+  friend struct F;
+  friend struct F;
+};
+
+} // namespace
+)";
+
+  testImport(Code, Lang_CXX11, "", Lang_CXX11, Verifier, namespaceDecl());
+}
+
+TEST(ImportExpr, ImportSpecializationsOfVariableTemplate) {
+  MatchVerifier Verifier;
+  auto Code =
+R"(
+namespace declToImport {
+
+struct limits {
+  template
+  static const T min; // declaration of a static data member template
+};
+template
+const T limits::min = { }; // definition of a static data member template
+
+template const int limits::min; // explicit instantiation
+
+} // namespace
+)";
+
+  testImport(Code, Lang_CXX14, "", Lang_CXX14, Verifier, namespaceDecl());
+}
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+  Decl *ToR1;
+  {
+Decl *FromTU = getTuDecl("struct A { friend class F0; friend class F1; };",
+ Lang_CXX, "input0.cc");
+auto *FromR = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("A")));
+
+ToR1 = Import(FromR, Lang_CXX);
+  }
+
+  Decl *ToR2;
+  {
+Decl *FromTU =
+getTuDecl("struct A { friend class F0; };", Lang_CXX, "input1.cc");
+auto *FromR = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("A")));
+
+ToR2 = Import(FromR, Lang_CXX);
+  }
+
+  EXPECT_NE(ToR1, ToR2);
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
   (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+static bool shouldBeHidden(NamedDecl *D);
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
  "decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
   }
 

[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 146765.
martong added a comment.

- Add FIXME


Repository:
  rC Clang

https://reviews.llvm.org/D46353

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
   MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
 }
 
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+  }
+
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B {
+  void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(
 ParameterizedTests, ASTImporterTestBase,
 ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4077,8 +4077,14 @@
   if (auto *FoundTemplate = dyn_cast(Found)) {
 if (IsStructuralMatch(D, FoundTemplate)) {
   // The class templates structurally match; call it the same template.
-  // FIXME: We may be filling in a forward declaration here. Handle
-  // this case!
+
+  // We found a forward declaration but the class to be imported has a
+  // definition.
+  // FIXME Add this forward declaration to the redeclaration chain.
+  if (D->isThisDeclarationADefinition() &&
+  !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
   Importer.Imported(D->getTemplatedDecl(), 
 FoundTemplate->getTemplatedDecl());
   return Importer.Imported(D, FoundTemplate);


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1431,6 +1431,39 @@
   MatchVerifier{}.match(To->getTranslationUnitDecl(), Pattern));
 }
 
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B;
+)",
+Lang_CXX, "input0.cc");
+auto *FromD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+
+Import(FromD, Lang_CXX);
+  }
+
+  {
+Decl *FromTU = getTuDecl(
+R"(
+template 
+struct B {
+  void f();
+};
+)",
+Lang_CXX, "input1.cc");
+FunctionDecl *FromD = FirstDeclMatcher().match(
+FromTU, functionDecl(hasName("f")));
+Import(FromD, Lang_CXX);
+auto *FromCTD = FirstDeclMatcher().match(
+FromTU, classTemplateDecl(hasName("B")));
+auto *ToCTD = cast(Import(FromCTD, Lang_CXX));
+EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(
 ParameterizedTests, ASTImporterTestBase,
 ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4077,8 +4077,14 @@
   if (auto *FoundTemplate = dyn_cast(Found)) {
 if (IsStructuralMatch(D, FoundTemplate)) {
   // The class templates structurally match; call it the same template.
-  // FIXME: We may be filling in a forward declaration here. Handle
-  // this case!
+
+  // We found a forward declaration but the class to be imported has a
+  // definition.
+  // FIXME Add this forward declaration to the redeclaration chain.
+  if (D->isThisDeclarationADefinition() &&
+  !FoundTemplate->isThisDeclarationADefinition())
+continue;
+
   Importer.Imported(D->getTemplatedDecl(), 
 FoundTemplate->getTemplatedDecl());
   return Importer.Imported(D, FoundTemplate);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46353: [ASTImporter] Extend lookup logic in class templates

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Hi Aleksei,

Added the FIXME, can you help me with committing this?


Repository:
  rC Clang

https://reviews.llvm.org/D46353



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


[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs, mgorny.

This patch add new tests for structural equivalence. For that a new common 
header is created which holds the test related language specific types and 
functions.


Repository:
  rC Clang

https://reviews.llvm.org/D46867

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/CMakeLists.txt
  unittests/AST/Language.h
  unittests/AST/MatchVerifier.h
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- /dev/null
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -0,0 +1,211 @@
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTImporter.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "Language.h"
+#include "DeclMatcher.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ast_matchers {
+
+struct StructuralEquivalenceTest : ::testing::Test {
+  std::unique_ptr AST0, AST1;
+  std::string Code0, Code1; // Buffers for SourceManager
+
+  // Get a pair of Decl pointers to the synthetised declarations from the given
+  // code snipets. By default we search for the unique Decl with name 'foo' in
+  // both snippets.
+  std::tuple
+  makeNamedDecls(const std::string &SrcCode0, const std::string &SrcCode1,
+ Language Lang, const char *const Identifier = "foo") {
+
+this->Code0 = SrcCode0;
+this->Code1 = SrcCode1;
+ArgVector Args = getBasicRunOptionsForLanguage(Lang);
+
+const char *const InputFileName = "input.cc";
+
+AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
+AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+
+ASTContext &Ctx0 = AST0->getASTContext(), &Ctx1 = AST1->getASTContext();
+
+auto getDecl = [](ASTContext &Ctx, const std::string &Name) -> NamedDecl * {
+  IdentifierInfo *ImportedII = &Ctx.Idents.get(Name);
+  assert(ImportedII && "Declaration with the identifier "
+   "should be specified in test!");
+  DeclarationName ImportDeclName(ImportedII);
+  SmallVector FoundDecls;
+  Ctx.getTranslationUnitDecl()->localUncachedLookup(ImportDeclName,
+FoundDecls);
+
+  // We should find one Decl but one only
+  assert(FoundDecls.size() > 0);
+  assert(FoundDecls.size() < 2);
+
+  return FoundDecls[0];
+};
+
+NamedDecl *d0 = getDecl(Ctx0, Identifier);
+NamedDecl *d1 = getDecl(Ctx1, Identifier);
+assert(d0);
+assert(d1);
+return std::make_tuple(d0, d1);
+  }
+
+  bool testStructuralMatch(NamedDecl *d0, NamedDecl *d1) {
+llvm::DenseSet> NonEquivalentDecls;
+StructuralEquivalenceContext Ctx(d0->getASTContext(), d1->getASTContext(),
+ NonEquivalentDecls, false, false);
+return Ctx.IsStructurallyEquivalent(d0, d1);
+  }
+};
+
+using std::get;
+
+TEST_F(StructuralEquivalenceTest, Int) {
+  auto t = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedInt) {
+  auto t = makeNamedDecls("int foo;", "signed int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, Char) {
+  auto t = makeNamedDecls("char foo;", "char foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+// This test is disabled for now.
+// FIXME Whether this is equivalent is dependendant on the target.
+TEST_F(StructuralEquivalenceTest, DISABLED_CharVsSignedChar) {
+  auto t = makeNamedDecls("char foo;", "signed char foo;", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, ForwardRecordDecl) {
+  auto t = makeNamedDecls("struct foo;", "struct foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntInStruct) {
+  auto t = makeNamedDecls("struct foo { int x; };",
+  "struct foo { signed int x; };", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, CharVsSignedCharInStruct) {
+  auto t = makeNamedDecls("struct foo { char x; };",
+  "struct foo { signed char x; };", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(get<0>(t), get<1>(t)));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntTemplateSpec) {
+  auto t = makeNamedDecls(
+  "template  struct foo; template<> struct foo{};",
+  "template  struct foo; template<> struct foo{};",
+  Lang_CXX);
+  ClassTemplateSpecializationDecl *Spec0 =
+  *cast(get<0>(t))->spec_beg

[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Hi Aleksei,

Thanks for reviewing this.
I could synthesize a test which exercises only the `DeclContext::removeDecl` 
function. This test causes an assertion without the fix.
Removed the rest of the testcases, which are not strictly connected to this 
change.




Comment at: unittests/AST/ASTImporterTest.cpp:1827
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportOfRecordWithDifferentFriends) {
+  Decl *ToR1;

a.sidorin wrote:
> For this change, we should create a separate patch.
This test is disabled ATM, but I agree it would be better to bring this in when 
we fix the import of friends.


Repository:
  rC Clang

https://reviews.llvm.org/D46835



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


[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 146845.
martong marked 4 inline comments as done.
martong added a comment.

- Add test for removeDecl, remove unrelated tests


Repository:
  rC Clang

https://reviews.llvm.org/D46835

Files:
  lib/AST/DeclBase.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/MatchVerifier.h

Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,11 +28,12 @@
 namespace clang {
 namespace ast_matchers {
 
-enum Language { 
+enum Language {
 Lang_C,
 Lang_C89,
 Lang_CXX,
 Lang_CXX11,
+Lang_CXX14,
 Lang_OpenCL,
 Lang_OBJCXX
 };
@@ -113,6 +114,10 @@
 Args.push_back("-std=c++11");
 FileName = "input.cc";
 break;
+  case Lang_CXX14:
+Args.push_back("-std=c++14");
+FileName = "input.cc";
+break;
   case Lang_OpenCL:
 FileName = "input.cl";
 break;
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -52,6 +52,9 @@
   case Lang_CXX11:
 BasicArgs = {"-std=c++11", "-frtti"};
 break;
+  case Lang_CXX14:
+BasicArgs = {"-std=c++14", "-frtti"};
+break;
   case Lang_OpenCL:
   case Lang_OBJCXX:
 llvm_unreachable("Not implemented yet!");
@@ -1764,5 +1767,38 @@
  compoundStmt(has(callExpr(has(unresolvedMemberExpr());
 }
 
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+  Decl *TU = getTuDecl(
+  R"(
+  namespace NS {
+
+  template 
+  struct S {};
+  template struct S;
+
+  inline namespace INS {
+template 
+struct S {};
+template struct S;
+  }
+
+  }
+  )", Lang_CXX11, "input0.cc");
+  auto *NS = FirstDeclMatcher().match(
+  TU, namespaceDecl());
+  auto *Spec = FirstDeclMatcher().match(
+  TU, classTemplateSpecializationDecl());
+  ASSERT_TRUE(NS->containsDecl(Spec));
+
+  NS->removeDecl(Spec);
+  EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
   (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+static bool shouldBeHidden(NamedDecl *D);
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
  "decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
   }
 }
   }
-  
+
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa(D)) {
 auto *ND = cast(D);
 
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup.  E.g. template specializations are skipped.
+if (shouldBeHidden(ND)) return;
+
 // Remove only decls that have a name
 if (!ND->getDeclName()) return;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-15 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 146847.
martong added a comment.

- Remove unrelated CXX14 changes


Repository:
  rC Clang

https://reviews.llvm.org/D46835

Files:
  lib/AST/DeclBase.cpp
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/MatchVerifier.h


Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,7 +28,7 @@
 namespace clang {
 namespace ast_matchers {
 
-enum Language { 
+enum Language {
 Lang_C,
 Lang_C89,
 Lang_CXX,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1764,5 +1764,38 @@
  compoundStmt(has(callExpr(has(unresolvedMemberExpr());
 }
 
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+  Decl *TU = getTuDecl(
+  R"(
+  namespace NS {
+
+  template 
+  struct S {};
+  template struct S;
+
+  inline namespace INS {
+template 
+struct S {};
+template struct S;
+  }
+
+  }
+  )", Lang_CXX11, "input0.cc");
+  auto *NS = FirstDeclMatcher().match(
+  TU, namespaceDecl());
+  auto *Spec = FirstDeclMatcher().match(
+  TU, classTemplateSpecializationDecl());
+  ASSERT_TRUE(NS->containsDecl(Spec));
+
+  NS->removeDecl(Spec);
+  EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
   (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+static bool shouldBeHidden(NamedDecl *D);
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
  "decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
   }
 }
   }
-  
+
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa(D)) {
 auto *ND = cast(D);
 
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup.  E.g. template specializations are skipped.
+if (shouldBeHidden(ND)) return;
+
 // Remove only decls that have a name
 if (!ND->getDeclName()) return;
 


Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -28,7 +28,7 @@
 namespace clang {
 namespace ast_matchers {
 
-enum Language { 
+enum Language {
 Lang_C,
 Lang_C89,
 Lang_CXX,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1764,5 +1764,38 @@
  compoundStmt(has(callExpr(has(unresolvedMemberExpr());
 }
 
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+  Decl *TU = getTuDecl(
+  R"(
+  namespace NS {
+
+  template 
+  struct S {};
+  template struct S;
+
+  inline namespace INS {
+template 
+struct S {};
+template struct S;
+  }
+
+  }
+  )", Lang_CXX11, "input0.cc");
+  auto *NS = FirstDeclMatcher().match(
+  TU, namespaceDecl());
+  auto *Spec = FirstDeclMatcher().match(
+  TU, classTemplateSpecializationDecl());
+  ASSERT_TRUE(NS->containsDecl(Spec));
+
+  NS->removeDecl(Spec);
+  EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,8 @@
   (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+static bool shouldBeHidden(NamedDecl *D);
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
  "decl being removed from non-lexical context");
@@ -1372,14 +1374,18 @@
   }
 }
   }
-  
+
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa(D)) {
 auto *ND = cast(D);
 
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup.  E.g. template specializations are s

[PATCH] D46950: Fix duplicate class template definitions problem

2018-05-16 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

We fail to import a `ClassTemplateDecl` if the "To" context already contains a 
definition and then a forward decl.
This is because `localUncachedLookup` does not find the definition.
This is not a lookup error, the parser behaves differently than assumed in the 
importer code.
A `DeclContext` contains one DenseMap (`LookupPtr`) which maps names to lists.
The list is a special list `StoredDeclsList` which is optimized to have one 
element.
During building the initial AST, the parser first adds the definition to the 
`DeclContext`.
Then during parsing the second declaration (the forward decl) the parser again 
calls `DeclContext::addDecl` but that will not add a new element to the 
`StoredDeclsList` rarther it simply overwrites the old element with the most 
recent one.
This patch fixes the error by finding the definition in the redecl chain.
Added tests for the same issue with `CXXRecordDecl` and with 
`ClassTemplateSpecializationDecl`.
These tests pass and they pass because in `VisitRecordDecl` and in 
`VisitClassTemplateSpecializationDecl` we already use `D->getDefinition()` 
after the lookup.


Repository:
  rC Clang

https://reviews.llvm.org/D46950

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

Index: unittests/AST/DeclMatcher.h
===
--- unittests/AST/DeclMatcher.h
+++ unittests/AST/DeclMatcher.h
@@ -44,24 +44,35 @@
 using FirstDeclMatcher = DeclMatcher;
 
 template 
-class DeclCounter : public MatchFinder::MatchCallback {
-  unsigned Count = 0;
+class DeclCounterWithPredicate : public MatchFinder::MatchCallback {
+  using UnaryPredicate = std::function;
+  UnaryPredicate predicate;
+  unsigned count = 0;
   void run(const MatchFinder::MatchResult &Result) override {
-  if(Result.Nodes.getNodeAs("")) {
-++Count;
-  }
+if (auto N = Result.Nodes.getNodeAs("")) {
+  if (predicate(N))
+++count;
+}
   }
+
 public:
-  // Returns the number of matched nodes under the tree rooted in `D`.
+  DeclCounterWithPredicate()
+  : predicate([](const NodeType *) { return true; }) {}
+  DeclCounterWithPredicate(UnaryPredicate p) : predicate(p) {}
+  // Returns the number of matched nodes which satisfy the predicate under the
+  // tree rooted in `D`.
   template 
   unsigned match(const Decl *D, const MatcherType &AMatcher) {
 MatchFinder Finder;
 Finder.addMatcher(AMatcher.bind(""), this);
 Finder.matchAST(D->getASTContext());
-return Count;
+return count;
   }
 };
 
+template 
+using DeclCounter = DeclCounterWithPredicate;
+
 } // end namespace ast_matchers
 } // end namespace clang
 
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -281,8 +281,9 @@
 return std::make_tuple(*FoundDecls.begin(), Imported);
   }
 
-  // Creates a TU decl for the given source code.
-  // May be called several times in a given test.
+  // Creates a TU decl for the given source code which can be used as a From
+  // context.  May be called several times in a given test (with different file
+  // name).
   TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang,
  StringRef FileName = "input.cc") {
 assert(
@@ -297,6 +298,16 @@
 return Tu.TUDecl;
   }
 
+  // Creates the To context with the given source code and returns the TU decl.
+  TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
+ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+ToCode = ToSrcCode;
+assert(!ToAST);
+ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+
+return ToAST->getASTContext().getTranslationUnitDecl();
+  }
+
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
@@ -1464,6 +1475,120 @@
   }
 }
 
+TEST_P(ASTImporterTestBase,
+   ImportDefinitionOfClassTemplateIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  struct B {
+void f();
+  };
+
+  template 
+  struct B;
+  )",
+  Lang_CXX);
+  ASSERT_EQ(1u, DeclCounterWithPredicate(
+[](const ClassTemplateDecl *T) {
+  return T->isThisDeclarationADefinition();
+})
+.match(ToTU, classTemplateDecl()));
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  struct B {
+void f();
+  };
+  )",
+  Lang_CXX, "input1.cc");
+  ClassTemplateDecl *FromD = FirstDeclMatcher().match(
+  FromTU, classTemplateDecl(hasName("B")));
+
+  Import(FromD, L

[PATCH] D46958: [ASTImporter] Fix missing implict CXXRecordDecl

2018-05-16 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: xazax.hun, a.sidorin, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Implicit CXXRecordDecl is not added to its DeclContext during import, but in
the original AST it is. This patch fixes this.


Repository:
  rC Clang

https://reviews.llvm.org/D46958

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1348,7 +1348,7 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_ShouldImportImplicitCXXRecordDecl) {
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2110,7 +2110,7 @@
   D2 = D2CXX;
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
-  if (!DCXX->getDescribedClassTemplate())
+  if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
 LexicalDC->addDeclInternal(D2);
 
   Importer.Imported(D, D2);


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1348,7 +1348,7 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_ShouldImportImplicitCXXRecordDecl) {
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2110,7 +2110,7 @@
   D2 = D2CXX;
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
-  if (!DCXX->getDescribedClassTemplate())
+  if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
 LexicalDC->addDeclInternal(D2);
 
   Importer.Imported(D, D2);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Hi Aleksei,

I am OK with this, I just have a little concern about friend declarations. 
Since https://reviews.llvm.org/D32947 ( [ASTImporter] FriendDecl importing 
improvements ) records' structural equivalency depends on the order of their 
friend declarations.
Anyway, I think friend related issues can be addressed in a separate patch.

What is a bit more concerning is that `*ToField` can be null and we should 
handle that.
This could happen if lookup finds the type of the field, but for some reason 
(e.g. there is an error in structural eq, or the redecl chain is incorrect) it 
turns out to be non-equivalent with the type of the FromField:

  for (auto *FoundDecl : FoundDecls) {
if (auto *FoundField = dyn_cast(FoundDecl)) {
  // For anonymous fields, match up by index.
  if (!Name && getFieldIndex(D) != getFieldIndex(FoundField))
continue;
  
  if (Importer.IsStructurallyEquivalent(D->getType(),
FoundField->getType())) {
Importer.Imported(D, FoundField);
return FoundField;
  }
  
  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
<< Name << D->getType() << FoundField->getType();
  Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
<< FoundField->getType();
  return nullptr;
}
  }






Comment at: lib/AST/ASTImporter.cpp:1025
+  // type import can depend on them.
+  const RecordDecl *FromRD = dyn_cast(FromDC);
+  if (!FromRD)

Could use `auto` here, to avoid redundant type specification.



Comment at: lib/AST/ASTImporter.cpp:1029
+
+  RecordDecl *ToRD = cast(Importer.Import(cast(FromDC)));
+

Can't we just import the `FromRD`, why we need that cast at the end? 
`auto *ToRD = cast(Importer.Import(FromRD)));` 



Comment at: lib/AST/ASTImporter.cpp:1032
+  for (auto *FromField : FromRD->fields()) {
+Decl *ToField = Importer.GetAlreadyImportedOrNull(FromField);
+assert(ToRD == ToField->getDeclContext() && ToRD->containsDecl(ToField));

I think `ToField` can be a nullptr, and if so, then `ToField->getDeclContext()` 
is UB.
Same could happen at line 1040.
Perhaps we should have and explicit check about the nullptr value:
`if (!ToField) continue;`




Comment at: unittests/AST/ASTImporterTest.cpp:1022
 
+AST_MATCHER_P(RecordDecl, hasFieldOrder, std::vector, Order) {
+  size_t Index = 0;

The `hasFieldOrder` matcher is already existing.



Comment at: unittests/AST/ASTImporterTest.cpp:1034
+
+TEST(ImportDecl, ImportFieldOrder) {
+  MatchVerifier Verifier;

We already have this test, we just have to enable it.
`DISABLED_CXXRecordDeclFieldOrderShouldNotDependOnImportOrder`


Repository:
  rC Clang

https://reviews.llvm.org/D44100



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


[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 147274.
martong added a comment.

Addressing review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D46835

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

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1797,5 +1797,38 @@
  compoundStmt(has(callExpr(has(unresolvedMemberExpr());
 }
 
+struct DeclContextTest : ASTImporterTestBase {};
+
+TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
+  Decl *TU = getTuDecl(
+  R"(
+  namespace NS {
+
+  template 
+  struct S {};
+  template struct S;
+
+  inline namespace INS {
+template 
+struct S {};
+template struct S;
+  }
+
+  }
+  )", Lang_CXX11, "input0.cc");
+  auto *NS = FirstDeclMatcher().match(
+  TU, namespaceDecl());
+  auto *Spec = FirstDeclMatcher().match(
+  TU, classTemplateSpecializationDecl());
+  ASSERT_TRUE(NS->containsDecl(Spec));
+
+  NS->removeDecl(Spec);
+  EXPECT_FALSE(NS->containsDecl(Spec));
+}
+
+INSTANTIATE_TEST_CASE_P(
+ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/AST/DeclBase.cpp
===
--- lib/AST/DeclBase.cpp
+++ lib/AST/DeclBase.cpp
@@ -1350,6 +1350,32 @@
   (D->NextInContextAndBits.getPointer() || D == LastDecl));
 }
 
+/// shouldBeHidden - Determine whether a declaration which was declared
+/// within its semantic context should be invisible to qualified name lookup.
+static bool shouldBeHidden(NamedDecl *D) {
+  // Skip unnamed declarations.
+  if (!D->getDeclName())
+return true;
+
+  // Skip entities that can't be found by name lookup into a particular
+  // context.
+  if ((D->getIdentifierNamespace() == 0 && !isa(D)) ||
+  D->isTemplateParameter())
+return true;
+
+  // Skip template specializations.
+  // FIXME: This feels like a hack. Should DeclarationName support
+  // template-ids, or is there a better way to keep specializations
+  // from being visible?
+  if (isa(D))
+return true;
+  if (auto *FD = dyn_cast(D))
+if (FD->isFunctionTemplateSpecialization())
+  return true;
+
+  return false;
+}
+
 void DeclContext::removeDecl(Decl *D) {
   assert(D->getLexicalDeclContext() == this &&
  "decl being removed from non-lexical context");
@@ -1372,16 +1398,22 @@
   }
 }
   }
-  
+
   // Mark that D is no longer in the decl chain.
   D->NextInContextAndBits.setPointer(nullptr);
 
   // Remove D from the lookup table if necessary.
   if (isa(D)) {
 auto *ND = cast(D);
 
+// Do not try to remove the declaration if that is invisible to qualified
+// lookup.  E.g. template specializations are skipped.
+if (shouldBeHidden(ND))
+  return;
+
 // Remove only decls that have a name
-if (!ND->getDeclName()) return;
+if (!ND->getDeclName())
+  return;
 
 auto *DC = D->getDeclContext();
 do {
@@ -1438,32 +1470,6 @@
 makeDeclVisibleInContextWithFlags(ND, true, true);
 }
 
-/// shouldBeHidden - Determine whether a declaration which was declared
-/// within its semantic context should be invisible to qualified name lookup.
-static bool shouldBeHidden(NamedDecl *D) {
-  // Skip unnamed declarations.
-  if (!D->getDeclName())
-return true;
-
-  // Skip entities that can't be found by name lookup into a particular
-  // context.
-  if ((D->getIdentifierNamespace() == 0 && !isa(D)) ||
-  D->isTemplateParameter())
-return true;
-
-  // Skip template specializations.
-  // FIXME: This feels like a hack. Should DeclarationName support
-  // template-ids, or is there a better way to keep specializations
-  // from being visible?
-  if (isa(D))
-return true;
-  if (auto *FD = dyn_cast(D))
-if (FD->isFunctionTemplateSpecialization())
-  return true;
-
-  return false;
-}
-
 /// buildLookup - Build the lookup data structure with all of the
 /// declarations in this DeclContext (and any other contexts linked
 /// to it or transparent contexts nested within it) and return it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46958: [ASTImporter] Fix missing implict CXXRecordDecl

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

In https://reviews.llvm.org/D46958#1101570, @a.sidorin wrote:

> So, we fail to add injected name to a CXXRecordDecl that has a described 
> class template?


Yes, we failed to import the implicit `CXXRecordDecl`.
Here is how it looked before the fix:

  From:
  ClassTemplateDecl 0xecae28  line:3:14 declToImport
  |-TemplateTypeParmDecl 0xecacd8  col:26 typename depth 0 
index 0 U
  `-CXXRecordDecl 0xecad90  line:3:14 struct declToImport 
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 0xecb0a0  col:14 implicit struct declToImport
  To:
  ClassTemplateDecl 0xf97cb0  col:14 declToImport
  |-TemplateTypeParmDecl 0xf97c00  col:26 typename depth 0 
index 0 U
  `-CXXRecordDecl 0xf97a48  col:14 struct declToImport 
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

Thanks for the review.


Repository:
  rC Clang

https://reviews.llvm.org/D46958



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


[PATCH] D46958: [ASTImporter] Fix missing implict CXXRecordDecl

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL332588: [ASTImporter] Fix missing implict CXXRecordDecl 
(authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D46958

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
@@ -2110,7 +2110,7 @@
   D2 = D2CXX;
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
-  if (!DCXX->getDescribedClassTemplate())
+  if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
 LexicalDC->addDeclInternal(D2);
 
   Importer.Imported(D, D2);
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1348,7 +1348,7 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_ShouldImportImplicitCXXRecordDecl) {
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2110,7 +2110,7 @@
   D2 = D2CXX;
   D2->setAccess(D->getAccess());
   D2->setLexicalDeclContext(LexicalDC);
-  if (!DCXX->getDescribedClassTemplate())
+  if (!DCXX->getDescribedClassTemplate() || DCXX->isImplicit())
 LexicalDC->addDeclInternal(D2);
 
   Importer.Imported(D, D2);
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1348,7 +1348,7 @@
   Verifier.match(To, cxxRecordDecl(hasFieldOrder({"a", "b", "c"};
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_ShouldImportImplicitCXXRecordDecl) {
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
martong marked 14 inline comments as done.
martong added a comment.

> Do you plan to enable this functionality for AST import checking?

Yes. We'd like to test the structural equivalency independently from 
ASTImporter, because in certain cases it may have faulty behavior. This can be 
very handy also when we further extend structural equivalency.




Comment at: unittests/AST/StructuralEquivalenceTest.cpp:119
+  *cast(get<1>(t))->spec_begin();
+  ASSERT_TRUE(Spec0 != nullptr);
+  ASSERT_TRUE(Spec1 != nullptr);

a.sidorin wrote:
> Should we assert for `spec_begin() != spec_end()` instead or within these 
> assertions?
I think that is not needed, because the previous `cast` in line 116 and 118 
would assert if `spec_begin() == spec_end()` was  true.



Comment at: unittests/AST/StructuralEquivalenceTest.cpp:174
+  )";
+  auto t = makeNamedDecls( Code0, Code0, Lang_CXX);
+

a.sidorin wrote:
> 1. It's better to use more meaningful names than `t`. DeclTuple?
> 2. The space after `(` is redundant.
Renamed `t` to `Decls`.


Repository:
  rC Clang

https://reviews.llvm.org/D46867



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


[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

2018-05-17 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 147304.
martong marked an inline comment as done.
martong added a comment.

- Address aleksei's comments


Repository:
  rC Clang

https://reviews.llvm.org/D46867

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/CMakeLists.txt
  unittests/AST/Language.cpp
  unittests/AST/Language.h
  unittests/AST/MatchVerifier.h
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- /dev/null
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -0,0 +1,208 @@
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "Language.h"
+#include "DeclMatcher.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ast_matchers {
+
+struct StructuralEquivalenceTest : ::testing::Test {
+  std::unique_ptr AST0, AST1;
+  std::string Code0, Code1; // Buffers for SourceManager
+
+  // Get a pair of Decl pointers to the synthetised declarations from the given
+  // code snipets. By default we search for the unique Decl with name 'foo' in
+  // both snippets.
+  std::tuple
+  makeNamedDecls(const std::string &SrcCode0, const std::string &SrcCode1,
+ Language Lang, const char *const Identifier = "foo") {
+
+this->Code0 = SrcCode0;
+this->Code1 = SrcCode1;
+ArgVector Args = getBasicRunOptionsForLanguage(Lang);
+
+const char *const InputFileName = "input.cc";
+
+AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
+AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+
+ASTContext &Ctx0 = AST0->getASTContext(), &Ctx1 = AST1->getASTContext();
+
+auto getDecl = [](ASTContext &Ctx, const std::string &Name) -> NamedDecl * {
+  IdentifierInfo *SearchedII = &Ctx.Idents.get(Name);
+  assert(SearchedII && "Declaration with the identifier "
+   "should be specified in test!");
+  DeclarationName SearchDeclName(SearchedII);
+  SmallVector FoundDecls;
+  Ctx.getTranslationUnitDecl()->localUncachedLookup(SearchDeclName,
+FoundDecls);
+
+  // We should find one Decl but one only.
+  assert(FoundDecls.size() == 1);
+
+  return FoundDecls[0];
+};
+
+NamedDecl *D0 = getDecl(Ctx0, Identifier);
+NamedDecl *D1 = getDecl(Ctx1, Identifier);
+assert(D0);
+assert(D1);
+return std::make_tuple(D0, D1);
+  }
+
+  bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
+llvm::DenseSet> NonEquivalentDecls;
+StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
+ NonEquivalentDecls, false, false);
+return Ctx.IsStructurallyEquivalent(D0, D1);
+  }
+
+  bool testStructuralMatch(std::tuple t) {
+using std::get;
+return testStructuralMatch(get<0>(t), get<1>(t));
+  }
+};
+
+using std::get;
+
+TEST_F(StructuralEquivalenceTest, Int) {
+  auto Decls = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedInt) {
+  auto Decls = makeNamedDecls("int foo;", "signed int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, Char) {
+  auto Decls = makeNamedDecls("char foo;", "char foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+// This test is disabled for now.
+// FIXME Whether this is equivalent is dependendant on the target.
+TEST_F(StructuralEquivalenceTest, DISABLED_CharVsSignedChar) {
+  auto Decls = makeNamedDecls("char foo;", "signed char foo;", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, ForwardRecordDecl) {
+  auto Decls = makeNamedDecls("struct foo;", "struct foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntInStruct) {
+  auto Decls = makeNamedDecls("struct foo { int x; };",
+  "struct foo { signed int x; };", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, CharVsSignedCharInStruct) {
+  auto Decls = makeNamedDecls("struct foo { char x; };",
+  "struct foo { signed char x; };", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntTemplateSpec) {
+  auto Decls = makeNamedDecls(
+  "template  struct foo; template<> struct foo{};",
+  "template  struct foo; template<> struct foo{};",
+  Lang_CXX);
+  ClassTemplateSpecializationDecl *Spec0 =
+  *cast(get<0>(Decls))->spec_begin();
+  ClassTemplateSpecializationDecl *Spec1 =
+  *cast(get<1>(Decls))->spec_begin();
+  ASSERT_TRUE(Spec0 != nullptr);
+  ASSERT_TRUE(Spec1 !

[PATCH] D46835: [ASTImporter] Do not try to remove invisible Decls from DeclContext

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong closed this revision.
martong added a comment.

Closed by commit: https://reviews.llvm.org/rL332699


Repository:
  rC Clang

https://reviews.llvm.org/D46835



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


[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Currently we do not import the implicit CXXRecordDecl of a
ClassTemplateSpecializationDecl. This patch fixes it.


Repository:
  rC Clang

https://reviews.llvm.org/D47057

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1367,7 +1367,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,7 +1959,7 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (!D->isImplicit() && Definition && Definition != D) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1367,7 +1367,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,7 +1959,7 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (!D->isImplicit() && Definition && Definition != D) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

ClassTemplateSpecialization is put in the wrong DeclContex if implicitly
instantiated. This patch fixes it.


Repository:
  rC Clang

https://reviews.llvm.org/D47058

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1246,7 +1246,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4298,9 +4298,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1246,7 +1246,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4298,9 +4298,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47069: [ASTImporter] Enable disabled but passing test

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

There is a test which passes since https://reviews.llvm.org/D32947, but it was 
forgotten to be enabled.
This patch enables that disabled test.


Repository:
  rC Clang

https://reviews.llvm.org/D47069

Files:
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1399,7 +1399,7 @@
   EXPECT_EQ(From->getIdentifierNamespace(), To->getIdentifierNamespace());
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_IDNSOfNonmemberOperator) {
+TEST_P(ASTImporterTestBase, IDNSOfNonmemberOperator) {
   Decl *FromTU = getTuDecl(
   R"(
   struct X {};


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1399,7 +1399,7 @@
   EXPECT_EQ(From->getIdentifierNamespace(), To->getIdentifierNamespace());
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_IDNSOfNonmemberOperator) {
+TEST_P(ASTImporterTestBase, IDNSOfNonmemberOperator) {
   Decl *FromTU = getTuDecl(
   R"(
   struct X {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:6776
+  // been invalidated to avoid repeatedly calling this.
+  ToContext.invalidateParents();
+

Can an `Expr` has a parent too? If yes, why not invalidate the parents in 
`Import(Expr*)` ?
I am also a bit concerned to call the invalidation during the import of all 
`Stmt` (and possible during the import of `Expr`s too).
Isn't it enough to invalidate only during `Import(Decl*)`?


https://reviews.llvm.org/D46940



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


[PATCH] D46940: [ASTImporter] make sure that ACtx::getParents still works

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

Ok, thanks for the explanation. Now it looks good to me.


https://reviews.llvm.org/D46940



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


[PATCH] D47069: [ASTImporter] Enable disabled but passing test

2018-05-18 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC332728: [ASTImporter] Enable disabled but passing test 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47069?vs=147506&id=147524#toc

Repository:
  rC Clang

https://reviews.llvm.org/D47069

Files:
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1399,7 +1399,7 @@
   EXPECT_EQ(From->getIdentifierNamespace(), To->getIdentifierNamespace());
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_IDNSOfNonmemberOperator) {
+TEST_P(ASTImporterTestBase, IDNSOfNonmemberOperator) {
   Decl *FromTU = getTuDecl(
   R"(
   struct X {};


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1399,7 +1399,7 @@
   EXPECT_EQ(From->getIdentifierNamespace(), To->getIdentifierNamespace());
 }
 
-TEST_P(ASTImporterTestBase, DISABLED_IDNSOfNonmemberOperator) {
+TEST_P(ASTImporterTestBase, IDNSOfNonmemberOperator) {
   Decl *FromTU = getTuDecl(
   R"(
   struct X {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

2018-05-22 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 147966.
martong marked an inline comment as done.
martong added a comment.

Use isExplicitInstantiationOrSpecialization


Repository:
  rC Clang

https://reviews.llvm.org/D47058

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1246,7 +1246,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4298,9 +4298,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->isExplicitInstantiationOrSpecialization()) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1246,7 +1246,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4298,9 +4298,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->isExplicitInstantiationOrSpecialization()) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

2018-05-22 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:4305
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->getTemplateSpecializationKind() != TSK_ImplicitInstantiation) {
+  LexicalDC->addDeclInternal(D2);

a.sidorin wrote:
> Can we write `if (D2->isExplicitInstantiationOrSpecialization())` instead? 
> How we should treat TSK_Undeclared case?
Yes, this is a good catch! Changed it.


Repository:
  rC Clang

https://reviews.llvm.org/D47058



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


[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-22 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:1962
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (!D->isImplicit() && Definition && Definition != D) {
 Decl *ImportedDef = Importer.Import(Definition);

a.sidorin wrote:
> We are changing import if RecordDecl. Is it possible to add a test that 
> doesn't require templates?
> I tried and found that the implicit CXXRecordDecl of 
> ClassTemplateSpecializationDecl is its redeclaration. That's not true for 
> normal CXXRecordDecls, as I see, so this deserves a comment.
Yes, I've added a new test which exercises only a RecordDecl.
Also, added a comment about the specialization re-declaration.


Repository:
  rC Clang

https://reviews.llvm.org/D47057



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


[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-22 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 147994.
martong marked an inline comment as done.
martong added a comment.

- Add new test case for simple CXXRecordDecl
- Add comment about ClassTemplateSpecializationDecl implicit CXXRecordDecl


Repository:
  rC Clang

https://reviews.llvm.org/D47057

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1352,6 +1352,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1367,7 +1383,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,19 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (!D->isImplicit() /*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.*/
+  && Definition && Definition != D) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1352,6 +1352,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1367,7 +1383,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,19 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (!D->isImplicit() /*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.*/
+  && Definition && Definition != D) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

2018-05-22 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 148013.
martong marked 5 inline comments as done.
martong added a comment.

- Addressing Aleksei's comments


Repository:
  rC Clang

https://reviews.llvm.org/D46950

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

Index: unittests/AST/DeclMatcher.h
===
--- unittests/AST/DeclMatcher.h
+++ unittests/AST/DeclMatcher.h
@@ -44,15 +44,23 @@
 using FirstDeclMatcher = DeclMatcher;
 
 template 
-class DeclCounter : public MatchFinder::MatchCallback {
+class DeclCounterWithPredicate : public MatchFinder::MatchCallback {
+  using UnaryPredicate = std::function;
+  UnaryPredicate Predicate;
   unsigned Count = 0;
   void run(const MatchFinder::MatchResult &Result) override {
-  if(Result.Nodes.getNodeAs("")) {
+if (auto N = Result.Nodes.getNodeAs("")) {
+  if (Predicate(N))
 ++Count;
-  }
+}
   }
+
 public:
-  // Returns the number of matched nodes under the tree rooted in `D`.
+  DeclCounterWithPredicate()
+  : Predicate([](const NodeType *) { return true; }) {}
+  DeclCounterWithPredicate(UnaryPredicate P) : Predicate(P) {}
+  // Returns the number of matched nodes which satisfy the predicate under the
+  // tree rooted in `D`.
   template 
   unsigned match(const Decl *D, const MatcherType &AMatcher) {
 MatchFinder Finder;
@@ -62,6 +70,9 @@
   }
 };
 
+template 
+using DeclCounter = DeclCounterWithPredicate;
+
 } // end namespace ast_matchers
 } // end namespace clang
 
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -281,8 +281,9 @@
 return std::make_tuple(*FoundDecls.begin(), Imported);
   }
 
-  // Creates a TU decl for the given source code.
-  // May be called several times in a given test.
+  // Creates a TU decl for the given source code which can be used as a From
+  // context.  May be called several times in a given test (with different file
+  // name).
   TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang,
  StringRef FileName = "input.cc") {
 assert(
@@ -297,6 +298,16 @@
 return Tu.TUDecl;
   }
 
+  // Creates the To context with the given source code and returns the TU decl.
+  TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
+ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+ToCode = ToSrcCode;
+assert(!ToAST);
+ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+
+return ToAST->getASTContext().getTranslationUnitDecl();
+  }
+
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
@@ -1464,6 +1475,121 @@
   }
 }
 
+TEST_P(ASTImporterTestBase,
+   ImportDefinitionOfClassTemplateIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  struct B {
+void f();
+  };
+
+  template 
+  struct B;
+  )",
+  Lang_CXX);
+  ASSERT_EQ(1u, DeclCounterWithPredicate(
+[](const ClassTemplateDecl *T) {
+  return T->isThisDeclarationADefinition();
+})
+.match(ToTU, classTemplateDecl()));
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  template 
+  struct B {
+void f();
+  };
+  )",
+  Lang_CXX, "input1.cc");
+  ClassTemplateDecl *FromD = FirstDeclMatcher().match(
+  FromTU, classTemplateDecl(hasName("B")));
+
+  Import(FromD, Lang_CXX);
+
+  // We should have only one definition.
+  EXPECT_EQ(1u, DeclCounterWithPredicate(
+[](const ClassTemplateDecl *T) {
+  return T->isThisDeclarationADefinition();
+})
+.match(ToTU, classTemplateDecl()));
+}
+
+TEST_P(ASTImporterTestBase,
+   ImportDefinitionOfClassIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
+  R"(
+  struct B {
+void f();
+  };
+
+  struct B;
+  )",
+  Lang_CXX);
+  ASSERT_EQ(2u, DeclCounter().match(
+ToTU, cxxRecordDecl(hasParent(translationUnitDecl();
+
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct B {
+void f();
+  };
+  )",
+  Lang_CXX, "input1.cc");
+  auto *FromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+
+  Import(FromD, Lang_CXX);
+
+  EXPECT_EQ(2u, DeclCounter().match(
+ToTU, cxxRecordDecl(hasParent(translationUnitDecl();
+}
+
+TEST_P(
+ASTImporterTestBase,
+ImportDefinitionOfClassTemplateSpecIfThereIsAnExistingFwdDeclAndDefinition)
+{
+  Decl *ToTU = getToTuDecl(
+  R"(
+  template 
+  struct B;
+
+  template <>
+  struct B {};
+

[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

2018-05-23 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333082: Fix duplicate class template definitions problem 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46950?vs=148013&id=148204#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46950

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

Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -4070,6 +4070,17 @@
   TemplateParams);
 }
 
+// Returns the definition for a (forward) declaration of a ClassTemplateDecl, if
+// it has any definition in the redecl chain.
+static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
+  CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+  if (!ToTemplatedDef)
+return nullptr;
+  ClassTemplateDecl *TemplateWithDef =
+  ToTemplatedDef->getDescribedClassTemplate();
+  return TemplateWithDef;
+}
+
 Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *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
@@ -4084,7 +4095,7 @@
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this class template.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
@@ -4103,34 +4114,39 @@
 for (auto *FoundDecl : FoundDecls) {
   if (!FoundDecl->isInIdentifierNamespace(Decl::IDNS_Ordinary))
 continue;
-  
+
   Decl *Found = FoundDecl;
   if (auto *FoundTemplate = dyn_cast(Found)) {
-if (IsStructuralMatch(D, FoundTemplate)) {
-  // The class templates structurally match; call it the same template.
 
-  // We found a forward declaration but the class to be imported has a
-  // definition.
-  // FIXME Add this forward declaration to the redeclaration chain.
-  if (D->isThisDeclarationADefinition() &&
-  !FoundTemplate->isThisDeclarationADefinition())
+// The class to be imported is a definition.
+if (D->isThisDeclarationADefinition()) {
+  // Lookup will find the fwd decl only if that is more recent than the
+  // definition. So, try to get the definition if that is available in
+  // the redecl chain.
+  ClassTemplateDecl *TemplateWithDef = getDefinition(FoundTemplate);
+  if (!TemplateWithDef)
 continue;
+  FoundTemplate = TemplateWithDef; // Continue with the definition.
+}
 
-  Importer.Imported(D->getTemplatedDecl(), 
+if (IsStructuralMatch(D, FoundTemplate)) {
+  // The class templates structurally match; call it the same template.
+
+  Importer.Imported(D->getTemplatedDecl(),
 FoundTemplate->getTemplatedDecl());
   return Importer.Imported(D, FoundTemplate);
-} 
+}
   }
-  
+
   ConflictingDecls.push_back(FoundDecl);
 }
-
+
 if (!ConflictingDecls.empty()) {
   Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
- ConflictingDecls.data(), 
+ ConflictingDecls.data(),
  ConflictingDecls.size());
 }
-
+
 if (!Name)
   return nullptr;
   }
Index: unittests/AST/DeclMatcher.h
===
--- unittests/AST/DeclMatcher.h
+++ unittests/AST/DeclMatcher.h
@@ -44,15 +44,23 @@
 using FirstDeclMatcher = DeclMatcher;
 
 template 
-class DeclCounter : public MatchFinder::MatchCallback {
+class DeclCounterWithPredicate : public MatchFinder::MatchCallback {
+  using UnaryPredicate = std::function;
+  UnaryPredicate Predicate;
   unsigned Count = 0;
   void run(const MatchFinder::MatchResult &Result) override {
-  if(Result.Nodes.getNodeAs("")) {
+if (auto N = Result.Nodes.getNodeAs("")) {
+  if (Predicate(N))
 ++Count;
-  }
+}
   }
+
 public:
-  // Returns the number of matched nodes under the tree rooted in `D`.
+  DeclCounterWithPredicate()
+  : Predicate([](const NodeType *) { return true; }) {}
+  DeclCounterWithPredicate(UnaryPredicate P) : Predicate(P) {}
+  // Returns the number of matched nodes which satisfy the predicate under the
+  // tree rooted in `D`.
   template 
   unsigned match(const Decl *D, const MatcherType &AMatcher) {
 MatchFinder Finder;
@@ -62,6 +70,9 @@
   }
 };
 
+template 
+using DeclCounter = DeclCounterWithPredicate;
+
 } // end namespace ast_matchers
 } // end namespace clang
 
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/AS

[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-23 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 148209.
martong added a comment.

Remove multiline comment and reorder parts of the condition


Repository:
  rC Clang

https://reviews.llvm.org/D47057

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-23 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333086: [ASTImporter] Fix missing implict CXXRecordDecl in… 
(authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47057

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
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47057: [ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl

2018-05-23 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333086: [ASTImporter] Fix missing implict CXXRecordDecl in… 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47057?vs=148209&id=148210#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47057

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1363,6 +1363,22 @@
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
+  struct declToImport {
+  };
+  )",
+  Lang_CXX, "", Lang_CXX);
+
+  MatchVerifier Verifier;
+  // Match the implicit Decl.
+  auto Matcher = cxxRecordDecl(has(cxxRecordDecl()));
+  ASSERT_TRUE(Verifier.match(From, Matcher));
+  EXPECT_TRUE(Verifier.match(To, Matcher));
+}
+
+TEST_P(ASTImporterTestBase, ShouldImportImplicitCXXRecordDeclOfClassTemplate) {
+  Decl *From, *To;
+  std::tie(From, To) = getImportedDecl(
+  R"(
   template 
   struct declToImport {
   };
@@ -1378,7 +1394,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
+ShouldImportImplicitCXXRecordDeclOfClassTemplateSpecializationDecl) {
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
   R"(
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -1959,14 +1959,20 @@
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   TagDecl *Definition = D->getDefinition();
-  if (Definition && Definition != D) {
+  if (Definition && Definition != D &&
+  // 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()) {
 Decl *ImportedDef = Importer.Import(Definition);
 if (!ImportedDef)
   return nullptr;
 
 return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this record.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

2018-05-24 Thread Gabor Marton via Phabricator via cfe-commits
martong updated this revision to Diff 148352.
martong added a comment.

Moved `using std::get` up, before `testStructuralMatch`.


Repository:
  rC Clang

https://reviews.llvm.org/D46867

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/CMakeLists.txt
  unittests/AST/Language.cpp
  unittests/AST/Language.h
  unittests/AST/MatchVerifier.h
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- /dev/null
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -0,0 +1,207 @@
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "Language.h"
+#include "DeclMatcher.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ast_matchers {
+
+using std::get;
+
+struct StructuralEquivalenceTest : ::testing::Test {
+  std::unique_ptr AST0, AST1;
+  std::string Code0, Code1; // Buffers for SourceManager
+
+  // Get a pair of Decl pointers to the synthetised declarations from the given
+  // code snipets. By default we search for the unique Decl with name 'foo' in
+  // both snippets.
+  std::tuple
+  makeNamedDecls(const std::string &SrcCode0, const std::string &SrcCode1,
+ Language Lang, const char *const Identifier = "foo") {
+
+this->Code0 = SrcCode0;
+this->Code1 = SrcCode1;
+ArgVector Args = getBasicRunOptionsForLanguage(Lang);
+
+const char *const InputFileName = "input.cc";
+
+AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
+AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+
+ASTContext &Ctx0 = AST0->getASTContext(), &Ctx1 = AST1->getASTContext();
+
+auto getDecl = [](ASTContext &Ctx, const std::string &Name) -> NamedDecl * {
+  IdentifierInfo *SearchedII = &Ctx.Idents.get(Name);
+  assert(SearchedII && "Declaration with the identifier "
+   "should be specified in test!");
+  DeclarationName SearchDeclName(SearchedII);
+  SmallVector FoundDecls;
+  Ctx.getTranslationUnitDecl()->localUncachedLookup(SearchDeclName,
+FoundDecls);
+
+  // We should find one Decl but one only.
+  assert(FoundDecls.size() == 1);
+
+  return FoundDecls[0];
+};
+
+NamedDecl *D0 = getDecl(Ctx0, Identifier);
+NamedDecl *D1 = getDecl(Ctx1, Identifier);
+assert(D0);
+assert(D1);
+return std::make_tuple(D0, D1);
+  }
+
+  bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
+llvm::DenseSet> NonEquivalentDecls;
+StructuralEquivalenceContext Ctx(D0->getASTContext(), D1->getASTContext(),
+ NonEquivalentDecls, false, false);
+return Ctx.IsStructurallyEquivalent(D0, D1);
+  }
+
+  bool testStructuralMatch(std::tuple t) {
+return testStructuralMatch(get<0>(t), get<1>(t));
+  }
+};
+
+TEST_F(StructuralEquivalenceTest, Int) {
+  auto Decls = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedInt) {
+  auto Decls = makeNamedDecls("int foo;", "signed int foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, Char) {
+  auto Decls = makeNamedDecls("char foo;", "char foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+// This test is disabled for now.
+// FIXME Whether this is equivalent is dependendant on the target.
+TEST_F(StructuralEquivalenceTest, DISABLED_CharVsSignedChar) {
+  auto Decls = makeNamedDecls("char foo;", "signed char foo;", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, ForwardRecordDecl) {
+  auto Decls = makeNamedDecls("struct foo;", "struct foo;", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntInStruct) {
+  auto Decls = makeNamedDecls("struct foo { int x; };",
+  "struct foo { signed int x; };", Lang_CXX);
+  EXPECT_TRUE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, CharVsSignedCharInStruct) {
+  auto Decls = makeNamedDecls("struct foo { char x; };",
+  "struct foo { signed char x; };", Lang_CXX);
+  EXPECT_FALSE(testStructuralMatch(Decls));
+}
+
+TEST_F(StructuralEquivalenceTest, IntVsSignedIntTemplateSpec) {
+  auto Decls = makeNamedDecls(
+  "template  struct foo; template<> struct foo{};",
+  "template  struct foo; template<> struct foo{};",
+  Lang_CXX);
+  ClassTemplateSpecializationDecl *Spec0 =
+  *cast(get<0>(Decls))->spec_begin();
+  ClassTemplateSpecializationDecl *Spec1 =
+  *cast(get<1>(Decls))->spec_begin();
+  ASSERT_TRUE(Spec0 != nullptr);
+  ASSERT_TRUE(Spec1 != nullptr);
+  EXPECT_TRUE(testStru

[PATCH] D46867: [ASTImporter] Add unit tests for structural equivalence

2018-05-24 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333166: [ASTImporter] Add unit tests for structural 
equivalence (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46867?vs=148352&id=148353#toc

Repository:
  rC Clang

https://reviews.llvm.org/D46867

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/CMakeLists.txt
  unittests/AST/Language.cpp
  unittests/AST/Language.h
  unittests/AST/MatchVerifier.h
  unittests/AST/StructuralEquivalenceTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -19,6 +19,7 @@
 #include "clang/Tooling/Tooling.h"
 
 #include "DeclMatcher.h"
+#include "Language.h"
 #include "gtest/gtest.h"
 #include "llvm/ADT/StringMap.h"
 
@@ -29,50 +30,6 @@
 using internal::BindableMatcher;
 using llvm::StringMap;
 
-typedef std::vector ArgVector;
-typedef std::vector RunOptions;
-
-static bool isCXX(Language Lang) {
-  return Lang == Lang_CXX || Lang == Lang_CXX11;
-}
-
-static ArgVector getBasicRunOptionsForLanguage(Language Lang) {
-  ArgVector BasicArgs;
-  // Test with basic arguments.
-  switch (Lang) {
-  case Lang_C:
-BasicArgs = {"-x", "c", "-std=c99"};
-break;
-  case Lang_C89:
-BasicArgs = {"-x", "c", "-std=c89"};
-break;
-  case Lang_CXX:
-BasicArgs = {"-std=c++98", "-frtti"};
-break;
-  case Lang_CXX11:
-BasicArgs = {"-std=c++11", "-frtti"};
-break;
-  case Lang_OpenCL:
-  case Lang_OBJCXX:
-llvm_unreachable("Not implemented yet!");
-  }
-  return BasicArgs;
-}
-
-static RunOptions getRunOptionsForLanguage(Language Lang) {
-  ArgVector BasicArgs = getBasicRunOptionsForLanguage(Lang);
-
-  // For C++, test with "-fdelayed-template-parsing" enabled to handle MSVC
-  // default behaviour.
-  if (isCXX(Lang)) {
-ArgVector ArgsForDelayedTemplateParse = BasicArgs;
-ArgsForDelayedTemplateParse.emplace_back("-fdelayed-template-parsing");
-return {BasicArgs, ArgsForDelayedTemplateParse};
-  }
-
-  return {BasicArgs};
-}
-
 // Creates a virtual file and assigns that to the context of given AST. If the
 // file already exists then the file will not be created again as a duplicate.
 static void
Index: unittests/AST/MatchVerifier.h
===
--- unittests/AST/MatchVerifier.h
+++ unittests/AST/MatchVerifier.h
@@ -23,20 +23,12 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
+#include "Language.h"
 #include "gtest/gtest.h"
 
 namespace clang {
 namespace ast_matchers {
 
-enum Language { 
-Lang_C,
-Lang_C89,
-Lang_CXX,
-Lang_CXX11,
-Lang_OpenCL,
-Lang_OBJCXX
-};
-
 /// \brief Base class for verifying some property of nodes found by a matcher.
 template 
 class MatchVerifier : public MatchFinder::MatchCallback {
@@ -113,6 +105,10 @@
 Args.push_back("-std=c++11");
 FileName = "input.cc";
 break;
+  case Lang_CXX14:
+Args.push_back("-std=c++14");
+FileName = "input.cc";
+break;
   case Lang_OpenCL:
 FileName = "input.cl";
 break;
Index: unittests/AST/CMakeLists.txt
===
--- unittests/AST/CMakeLists.txt
+++ unittests/AST/CMakeLists.txt
@@ -15,9 +15,11 @@
   DeclTest.cpp
   EvaluateAsRValueTest.cpp
   ExternalASTSourceTest.cpp
+  Language.cpp
   NamedDeclPrinterTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
+  StructuralEquivalenceTest.cpp
   )
 
 target_link_libraries(ASTTests
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -0,0 +1,207 @@
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Tooling/Tooling.h"
+
+#include "Language.h"
+#include "DeclMatcher.h"
+
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ast_matchers {
+
+using std::get;
+
+struct StructuralEquivalenceTest : ::testing::Test {
+  std::unique_ptr AST0, AST1;
+  std::string Code0, Code1; // Buffers for SourceManager
+
+  // Get a pair of Decl pointers to the synthetised declarations from the given
+  // code snipets. By default we search for the unique Decl with name 'foo' in
+  // both snippets.
+  std::tuple
+  makeNamedDecls(const std::string &SrcCode0, const std::string &SrcCode1,
+ Language Lang, const char *const Identifier = "foo") {
+
+this->Code0 = SrcCode0;
+this->Code1 = SrcCode1;
+ArgVector Args = getBasicRunOptionsForLanguage(Lang);
+
+const char *const InputFileName = "input.cc";
+
+AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args,

[PATCH] D47313: [ASTImporter] Corrected lookup at import of templated record decl

2018-05-24 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Looks good to me, but let's wait for Aleksei's approval and comments.


Repository:
  rC Clang

https://reviews.llvm.org/D47313



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


[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

2018-05-24 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> Could you add a test for TSK_Undeclared as well?

TLDR: No, I cannot.
`TSK_Undeclared` is used to indicate that a template specialization was formed 
from a template-id but has not yet been declared, defined, or instantiated.
Consequently, 
`ClassTemplateSpecializationDecl::ClassTemplateSpecializationDecl` and 
`VarTemplateSpecializationDecl::VarTemplateSpecializationDecl` initializes its 
member `SpecializationKind` to the value `TSK_Undeclared`.
`SpecializationKind` is getting set during parsing to a meaningful kind, it is 
never left as `TSK_Undeclared`.
So, the only way I could write a test is to manually change the 
`SpecializationKind` in the "From" context, but I consider that as not a 
meaningful test, since we cannot create such `Decl` by the parser.


Repository:
  rC Clang

https://reviews.llvm.org/D47058



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


[PATCH] D47058: [ASTImporter] Fix ClassTemplateSpecialization in wrong DC

2018-05-25 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL333269: [ASTImporter] Fix ClassTemplateSpecialization in 
wrong DC (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D47058

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
@@ -4320,9 +4320,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->isExplicitInstantiationOrSpecialization()) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1214,7 +1214,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-
DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -4320,9 +4320,13 @@
 
 D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
 
-// Add the specialization to this context.
+// Set the context of this specialization/instantiation.
 D2->setLexicalDeclContext(LexicalDC);
-LexicalDC->addDeclInternal(D2);
+
+// Add to the DC only if it was an explicit specialization/instantiation.
+if (D2->isExplicitInstantiationOrSpecialization()) {
+  LexicalDC->addDeclInternal(D2);
+}
   }
   Importer.Imported(D, D2);
   if (D->isCompleteDefinition() && ImportDefinition(D, D2))
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -1214,7 +1214,7 @@
 
 TEST_P(
 ASTImporterTestBase,
-DISABLED_TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
+TUshouldNotContainClassTemplateSpecializationOfImplicitInstantiation) {
 
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase

2018-05-25 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

In order to avoid build failures on MS, we use -fms-compatibility too in the
tests which use the TestBase.


Repository:
  rC Clang

https://reviews.llvm.org/D47367

Files:
  unittests/AST/ASTImporterTest.cpp


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1563,10 +1563,6 @@
 .match(ToTU, classTemplateSpecializationDecl()));
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, ASTImporterTestBase,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
-
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
@@ -1791,10 +1787,6 @@
   EXPECT_TRUE(To->isVirtual());
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, ImportFunctions,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
-
 AST_MATCHER_P(TagDecl, hasTypedefForAnonDecl, Matcher,
   InnerMatcher) {
   if (auto *Typedef = Node.getTypedefNameForAnonDecl())
@@ -1925,9 +1917,20 @@
   EXPECT_FALSE(NS->containsDecl(Spec));
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector()), );
+
+auto DefaultTestValuesForRunOptions = ::testing::Values(
+ArgVector(),
+ArgVector{"-fdelayed-template-parsing"},
+ArgVector{"-fms-compatibility"},
+ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterTestBase,
+DefaultTestValuesForRunOptions, );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
+DefaultTestValuesForRunOptions, );
 
 } // end namespace ast_matchers
 } // end namespace clang


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1563,10 +1563,6 @@
 .match(ToTU, classTemplateSpecializationDecl()));
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, ASTImporterTestBase,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
-
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
@@ -1791,10 +1787,6 @@
   EXPECT_TRUE(To->isVirtual());
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, ImportFunctions,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
-
 AST_MATCHER_P(TagDecl, hasTypedefForAnonDecl, Matcher,
   InnerMatcher) {
   if (auto *Typedef = Node.getTypedefNameForAnonDecl())
@@ -1925,9 +1917,20 @@
   EXPECT_FALSE(NS->containsDecl(Spec));
 }
 
-INSTANTIATE_TEST_CASE_P(
-ParameterizedTests, DeclContextTest,
-::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
+::testing::Values(ArgVector()), );
+
+auto DefaultTestValuesForRunOptions = ::testing::Values(
+ArgVector(),
+ArgVector{"-fdelayed-template-parsing"},
+ArgVector{"-fms-compatibility"},
+ArgVector{"-fdelayed-template-parsing", "-fms-compatibility"});
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterTestBase,
+DefaultTestValuesForRunOptions, );
+
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportFunctions,
+DefaultTestValuesForRunOptions, );
 
 } // end namespace ast_matchers
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D46950: [ASTImporter] Fix duplicate class template definitions problem

2018-05-25 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

@a.sidorin 
Yes, that is a very good idea.
Just created a new patch which adds that switch for the tests which use the 
TestBase.
https://reviews.llvm.org/D47367


Repository:
  rC Clang

https://reviews.llvm.org/D46950



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


[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase

2018-05-25 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

This patch does not target `testImport` because I am not sure how to handle the 
options there:

  auto RunOptsFrom = getRunOptionsForLanguage(FromLang);
  auto RunOptsTo = getRunOptionsForLanguage(ToLang);
  for (const auto &FromArgs : RunOptsFrom)
for (const auto &ToArgs : RunOptsTo)
  EXPECT_TRUE(testImport(FromCode, FromArgs, ToCode, ToArgs,
 Verifier, AMatcher));

I think, it is overkill to test all possible combinations of the options, we 
should come up with something better here.
Perhaps we could exploit parameterized tests here as well. @a.sidorin, 
@xazax.hun What is your opinion?


Repository:
  rC Clang

https://reviews.llvm.org/D47367



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


[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.

2018-05-28 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM! But let's wait for someone else's review too.

@a.sidorin 
We discovered this error during the CTU analysis of google/protobuf and we 
could reduce the erroneous C file with c-reduce to the minimal example 
presented in the test file.


Repository:
  rC Clang

https://reviews.llvm.org/D47450



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


[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.

2018-05-28 Thread Gabor Marton via Phabricator via cfe-commits
martong added a reviewer: r.stahl.
martong added a comment.

Adding Rafael too as a reviewer, because he has been working also on the 
ASTImporter recently.


Repository:
  rC Clang

https://reviews.llvm.org/D47450



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


[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

2018-05-28 Thread Gabor Marton via Phabricator via cfe-commits
martong added reviewers: akyrtzi, ddunbar.
martong added a comment.

Adding Argyrios and Daniel as a reviewer for ASTUnit related changes.


Repository:
  rC Clang

https://reviews.llvm.org/D47445



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


[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.

2018-05-28 Thread Gabor Marton via Phabricator via cfe-commits
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.

LGTM! But let's wait the review of those people who have history in 
`ASTUnit.cpp`.


Repository:
  rC Clang

https://reviews.llvm.org/D47445



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


[PATCH] D47367: [ASTImporter] Add ms compatibility to tests which use the TestBase

2018-05-29 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> I agree with that. I think we need to test just import pairs 
> {/*From*/no_option, /*To*/no_option}, {option_1, option1}, {option_2, 
> option_2}, ...{option_n, option_n}.

This patch does exactly that with the parameterized tests. Each elements of the 
`DefaultTestValuesForRunOptions` will be used both for the "To" and for the 
"From" context.
So we will have each TEST_P generating 4 cases:
[FROM, TO]
{no_option, no_option}
{"-fdelayed-template-parsing", "-fdelayed-template-parsing"}
{"-fms-compatibility", "-fms-compatibility"}
{"-fdelayed-template-parsing -fms-compatibility", "-fdelayed-template-parsing 
-fms-compatibility"}


Repository:
  rC Clang

https://reviews.llvm.org/D47367



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


[PATCH] D47313: [ASTImporter] Corrected lookup at import of templated record decl

2018-05-30 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Balazs, I'll commit it for you in an hour.


Repository:
  rC Clang

https://reviews.llvm.org/D47313



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


[PATCH] D47313: [ASTImporter] Corrected lookup at import of templated record decl

2018-05-30 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rC333522: [ASTImporter] Corrected lookup at import of 
templated record decl (authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D47313?vs=148361&id=149065#toc

Repository:
  rC Clang

https://reviews.llvm.org/D47313

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


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2015,7 +2015,14 @@
 if (const auto *Tag = Typedef->getUnderlyingType()->getAs())
   Found = Tag->getDecl();
   }
-  
+
+  if (D->getDescribedTemplate()) {
+if (auto *Template = dyn_cast(Found))
+  Found = Template->getTemplatedDecl();
+else
+  continue;
+  }
+
   if (auto *FoundRecord = dyn_cast(Found)) {
 if (!SearchName) {
   // If both unnamed structs/unions are in a record context, make sure
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1107,6 +1107,50 @@
  has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfClassTemplateDecl) {
+  Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);
+  auto From =
+  FirstDeclMatcher().match(FromTU, classTemplateDecl());
+  ASSERT_TRUE(From);
+  auto To = cast(Import(From, Lang_CXX));
+  ASSERT_TRUE(To);
+  Decl *ToTemplated = To->getTemplatedDecl();
+  Decl *ToTemplated1 = Import(From->getTemplatedDecl(), Lang_CXX);
+  EXPECT_TRUE(ToTemplated1);
+  EXPECT_EQ(ToTemplated1, ToTemplated);
+}
+
+TEST_P(ASTImporterTestBase, ImportCorrectTemplatedDecl) {
+  auto Code =
+R"(
+namespace x {
+  template struct S1{};
+  template struct S2{};
+  template struct S3{};
+}
+)";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto FromNs =
+  FirstDeclMatcher().match(FromTU, namespaceDecl());
+  auto ToNs = cast(Import(FromNs, Lang_CXX));
+  ASSERT_TRUE(ToNs);
+  auto From =
+  FirstDeclMatcher().match(FromTU,
+  classTemplateDecl(
+  hasName("S2")));
+  auto To =
+  FirstDeclMatcher().match(ToNs,
+  classTemplateDecl(
+  hasName("S2")));
+  ASSERT_TRUE(From);
+  ASSERT_TRUE(To);
+  auto ToTemplated = To->getTemplatedDecl();
+  auto ToTemplated1 =
+  cast(Import(From->getTemplatedDecl(), Lang_CXX));
+  EXPECT_TRUE(ToTemplated1);
+  ASSERT_EQ(ToTemplated1, ToTemplated);
+}
+
 TEST_P(ASTImporterTestBase, DISABLED_ImportFunctionWithBackReferringParameter) 
{
   Decl *From, *To;
   std::tie(From, To) = getImportedDecl(


Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -2015,7 +2015,14 @@
 if (const auto *Tag = Typedef->getUnderlyingType()->getAs())
   Found = Tag->getDecl();
   }
-  
+
+  if (D->getDescribedTemplate()) {
+if (auto *Template = dyn_cast(Found))
+  Found = Template->getTemplatedDecl();
+else
+  continue;
+  }
+
   if (auto *FoundRecord = dyn_cast(Found)) {
 if (!SearchName) {
   // If both unnamed structs/unions are in a record context, make sure
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -1107,6 +1107,50 @@
  has(fieldDecl(hasType(dependentSizedArrayType(;
 }
 
+TEST_P(ASTImporterTestBase, ImportOfTemplatedDeclOfClassTemplateDecl) {
+  Decl *FromTU = getTuDecl("template struct S{};", Lang_CXX);
+  auto From =
+  FirstDeclMatcher().match(FromTU, classTemplateDecl());
+  ASSERT_TRUE(From);
+  auto To = cast(Import(From, Lang_CXX));
+  ASSERT_TRUE(To);
+  Decl *ToTemplated = To->getTemplatedDecl();
+  Decl *ToTemplated1 = Import(From->getTemplatedDecl(), Lang_CXX);
+  EXPECT_TRUE(ToTemplated1);
+  EXPECT_EQ(ToTemplated1, ToTemplated);
+}
+
+TEST_P(ASTImporterTestBase, ImportCorrectTemplatedDecl) {
+  auto Code =
+R"(
+namespace x {
+  template struct S1{};
+  template struct S2{};
+  template struct S3{};
+}
+)";
+  Decl *FromTU = getTuDecl(Code, Lang_CXX);
+  auto FromNs =
+  FirstDeclMatcher().match(FromTU, namespaceDecl());
+  auto ToNs = cast(Import(FromNs, Lang_CXX));
+  ASSERT_TRUE(ToNs);
+  auto From =
+  FirstDeclMatcher().match(FromTU,
+  classTemplateD

[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation

2017-10-10 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/StaticAnalyzer/Core/IssueHash.cpp:39
+  // primary template.
+  if (const FunctionDecl *InstantiatedFrom =
+  Target->getInstantiatedFromMemberFunction())

Could we use here FunctionDecl::getPrimaryTemplate() ? That seems more general, 
it handles both specializations and instantiations.



Comment at: test/Analysis/bug_hash_test.cpp:61
 
-// CHECK: diagnostics
+template 
+T f(T i) {

We could add a few more test cases:
- a function template in class template
- specializations vs instantiations
- the combination of the above two (?)




Comment at: test/Analysis/bug_hash_test.cpp:1363
 // CHECK-NEXT:  
+// CHECK-NEXT:  
+// CHECK-NEXT:   path

I am not sure if this is possible, but could we add unit test just for the 
`GetSignature` function? Instead of these huge plist files?

I am thinking something like this:
https://github.com/martong/friend-stats/blob/ed0c69ea3669c933204c799f59b85cd7b2507c34/ut/FriendFunctionsTest.cpp#L31


Repository:
  rL LLVM

https://reviews.llvm.org/D38728



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


[PATCH] D38728: [analyzer] Use the signature of the primary template for issue hash calculation

2017-10-11 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: test/Analysis/bug_hash_test.cpp:105
+void g() {
+  TX x;
+  TX xl;

As we discussed, the checking of the equality of the `IssueString` in case of 
`TX` and `TX` is implicit. And as such it is hard to see that it 
is really tested. Perhaps a comment here could indicate this implicit checking 
mechanism.


https://reviews.llvm.org/D38728



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


[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

2017-10-20 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Consider the use of a function pointer:

  void* malloc(int);
  int strlen(char*);
  auto fp = malloc;
  void bad_malloc(char *str) { char *c = (char *)fp(strlen(str + 1)); }

I think, the checker will not match in this case.

One might use allocation functions via a function pointer in case of more 
possible allocation strategies (e.g having a different strategy for a shared 
memory allocation).


https://reviews.llvm.org/D39121



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


[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

2017-10-20 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

We might get false positives in case of certain substring operations.
Consider the case of copying a substring, pseudo code below:

  const char * s = "abcdefg";
  int offset = my_find('d', s);
  // I want to copy "defg"
  char *new_subststring = (char*) malloc(strlen(s + offset));
  strcpy(...);


https://reviews.llvm.org/D39121



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


[PATCH] D51533: [ASTImporter] Merge ExprBits

2018-08-31 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a_sidorin, r.stahl, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.

Some `Expr` classes set up default values for the `ExprBits` of `Stmt`.  These
default values are then overwritten by the parser sometimes.  One example is
`InitListExpr` which sets the value kind to be an rvalue in the ctor.  However,
this bit may change after the `InitListExpr` is created.  There may be other
expressions similar to `InitListExpr` in this sense, thus the safest solution
is to copy the expression bits.

The lack of copying `ExprBits` causes an assertion in the analyzer engine in a
specific case: Since the value kind is not imported, the analyzer engine
believes that the given InitListExpr is an rvalue, thus it creates a
nonloc::CompoundVal instead of creating memory region (as in case of an lvalue
reference).


Repository:
  rC Clang

https://reviews.llvm.org/D51533

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


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
   
unless(classTemplatePartialSpecializationDecl();
 }
 
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+  Decl *TU = getTuDecl(
+  R"(
+  const int &init();
+  void foo() { const int &a{init()}; }
+  )", Lang_CXX11, "input0.cc");
+  auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+  ASSERT_TRUE(FromD->getAnyInitializer());
+  auto *InitExpr = FromD->getAnyInitializer();
+  ASSERT_TRUE(InitExpr);
+  ASSERT_TRUE(InitExpr->isGLValue());
+
+  auto *ToD = Import(FromD, Lang_CXX11);
+  ASSERT_TRUE(ToD);
+  auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+  ASSERT_TRUE(ToInitExpr);
+  EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -6849,9 +6849,9 @@
 To->setSyntacticForm(ToSyntForm);
   }
 
+  // Copy InitListExprBitfields, which are not handled in the ctor of
+  // InitListExpr.
   To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
-  To->setValueDependent(ILE->isValueDependent());
-  To->setInstantiationDependent(ILE->isInstantiationDependent());
 
   return To;
 }
@@ -7196,6 +7196,19 @@
   if (!ToS)
 return nullptr;
 
+  if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+  }
+
   // Record the imported declaration.
   ImportedStmts[FromS] = ToS;
   return ToS;


Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
   unless(classTemplatePartialSpecializationDecl();
 }
 
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+  Decl *TU = getTuDecl(
+  R"(
+  const int &init();
+  void foo() { const int &a{init()}; }
+  )", Lang_CXX11, "input0.cc");
+  auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+  ASSERT_TRUE(FromD->getAnyInitializer());
+  auto *InitExpr = FromD->getAnyInitializer();
+  ASSERT_TRUE(InitExpr);
+  ASSERT_TRUE(InitExpr->isGLValue());
+
+  auto *ToD = Import(FromD, Lang_CXX11);
+  ASSERT_TRUE(ToD);
+  auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+  ASSERT_TRUE(ToInitExpr);
+  EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
Index: lib/AST/ASTImporter.cpp
===
--- lib/AST/ASTImporter.cpp
+++ lib/AST/ASTImporter.cpp
@@ -6849,9 +6849,9 @@
 To->setSyntacticForm(ToSyntForm);
   }
 
+  // Copy InitListExprBitfields, which are not handled in the ctor of
+  // InitListExpr.
   To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
-  To->setValueDependent(ILE->isValueDependent());
-  To->setInstantiationDependent(ILE->isInstantiationDependent());
 
   return To;
 }
@@ -7196,6 +7196,19 @@
   if (!ToS)
 return nullptr;
 
+  if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+  

[PATCH] D51533: [ASTImporter] Merge ExprBits

2018-09-03 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:3241
+  auto *ToD = Import(FromD, Lang_CXX11);
+  ASSERT_TRUE(ToD);
+  auto *ToInitExpr = cast(ToD)->getAnyInitializer();

a_sidorin wrote:
> EXPECT_TRUE (same below).
Thanks, changed it.


Repository:
  rL LLVM

https://reviews.llvm.org/D51533



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


[PATCH] D51533: [ASTImporter] Merge ExprBits

2018-09-03 Thread 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 rL341316: [ASTImporter] Merge ExprBits (authored by martong, 
committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51533?vs=163494&id=163707#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51533

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
@@ -6817,9 +6817,9 @@
 To->setSyntacticForm(ToSyntForm);
   }
 
+  // Copy InitListExprBitfields, which are not handled in the ctor of
+  // InitListExpr.
   To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
-  To->setValueDependent(ILE->isValueDependent());
-  To->setInstantiationDependent(ILE->isInstantiationDependent());
 
   return To;
 }
@@ -7164,6 +7164,19 @@
   if (!ToS)
 return nullptr;
 
+  if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+  }
+
   // Record the imported declaration.
   ImportedStmts[FromS] = ToS;
   return ToS;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
   
unless(classTemplatePartialSpecializationDecl();
 }
 
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+  Decl *TU = getTuDecl(
+  R"(
+  const int &init();
+  void foo() { const int &a{init()}; }
+  )", Lang_CXX11, "input0.cc");
+  auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+  ASSERT_TRUE(FromD->getAnyInitializer());
+  auto *InitExpr = FromD->getAnyInitializer();
+  ASSERT_TRUE(InitExpr);
+  ASSERT_TRUE(InitExpr->isGLValue());
+
+  auto *ToD = Import(FromD, Lang_CXX11);
+  EXPECT_TRUE(ToD);
+  auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+  EXPECT_TRUE(ToInitExpr);
+  EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {


Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -6817,9 +6817,9 @@
 To->setSyntacticForm(ToSyntForm);
   }
 
+  // Copy InitListExprBitfields, which are not handled in the ctor of
+  // InitListExpr.
   To->sawArrayRangeDesignator(ILE->hadArrayRangeDesignator());
-  To->setValueDependent(ILE->isValueDependent());
-  To->setInstantiationDependent(ILE->isInstantiationDependent());
 
   return To;
 }
@@ -7164,6 +7164,19 @@
   if (!ToS)
 return nullptr;
 
+  if (auto *ToE = dyn_cast(ToS)) {
+auto *FromE = cast(FromS);
+// Copy ExprBitfields, which may not be handled in Expr subclasses
+// constructors.
+ToE->setValueKind(FromE->getValueKind());
+ToE->setObjectKind(FromE->getObjectKind());
+ToE->setTypeDependent(FromE->isTypeDependent());
+ToE->setValueDependent(FromE->isValueDependent());
+ToE->setInstantiationDependent(FromE->isInstantiationDependent());
+ToE->setContainsUnexpandedParameterPack(
+FromE->containsUnexpandedParameterPack());
+  }
+
   // Record the imported declaration.
   ImportedStmts[FromS] = ToS;
   return ToS;
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -3225,6 +3225,25 @@
   unless(classTemplatePartialSpecializationDecl();
 }
 
+TEST_P(ASTImporterTestBase, InitListExprValueKindShouldBeImported) {
+  Decl *TU = getTuDecl(
+  R"(
+  const int &init();
+  void foo() { const int &a{init()}; }
+  )", Lang_CXX11, "input0.cc");
+  auto *FromD = FirstDeclMatcher().match(TU, varDecl(hasName("a")));
+  ASSERT_TRUE(FromD->getAnyInitializer());
+  auto *InitExpr = FromD->getAnyInitializer();
+  ASSERT_TRUE(InitExpr);
+  ASSERT_TRUE(InitExpr->isGLValue());
+
+  auto *ToD = Import(FromD, Lang_CXX11);
+  EXPECT_TRUE(ToD);
+  auto *ToInitExpr = cast(ToD)->getAnyInitializer();
+  EXPECT_TRUE(ToInitExpr);
+  EXPECT_TRUE(ToInitExpr->isGLValue());
+}
+
 str

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

2018-09-03 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a_sidorin, xazax.hun, r.stahl.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: a.sidorin.

The init expression of a VarDecl is overwritten in the "To" context if we
import a VarDecl without an init expression (and with a definition).  Please
refer to the added tests, especially InitAndDefinitionAreInDifferentTUs.  This
patch fixes the malfunction by importing the whole Decl chain similarly as we
did that in case of FunctionDecls.  We handle the init expression similarly to
a  definition, alas only one init expression will be in the merged ast.


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::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());
+  EX

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

2018-09-03 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:3763
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods,
+DefaultTestValuesForRunOptions, );

This hunk has nothing to do with this change, but previously we forgot to 
instantiate these test cases :(



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] D50672: [ASTImporter] Change the return result of Decl import to Optional

2018-09-04 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Here is the `Expected` based patch: https://reviews.llvm.org/D51633


Repository:
  rC Clang

https://reviews.llvm.org/D50672



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


[PATCH] D51633: [ASTImporter] Added error handling for AST import.

2018-09-04 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

This patch is huge, but we change here almost all implementation functions of 
`ASTNodeImporter` to return with either `Error` or with `Expected`.
We could not really come up with a cohesive but smaller patch because of the 
recursive nature of the importer. (We are open to ideas about how to cut this 
patch into smaller parts.)
Most of the changes are trivial: we always have to check the return value of 
any import function and pass on the error to the caller if there was any.
In my opinion one great simplification is the introduction of the auxiliary 
`importSeq` function. It imports each entity one-by-one after each other and in 
case of any error it returns with that error and does not continue with the 
subsequent import operations.


Repository:
  rC Clang

https://reviews.llvm.org/D51633



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


[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

2018-09-05 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

From this little information I have hear are my thoughts:

> match callExpr(allOf (callee(functionDecl(hasName("std::sort"))), 
> hasArgument(0,
> hasDescendant(declRefExpr(to(fieldDecl(hasName("value_type"))

I think this is a good direction, but keep in mind that `value_type` is a 
typedef, thus you should use the `typedefNameDecl` matcher instead of the 
`fieldDecl`.

(Also if I understand correctly then this is good that this matcher does not 
match in case of the `intPointerArray` example, because the array does not have 
any member at all ...)


Repository:
  rC Clang

https://reviews.llvm.org/D50488



___
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

2018-09-12 Thread 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

2018-09-12 Thread 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::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] D49223: [AST] Check described template at structural equivalence check.

2018-07-24 Thread Gabor Marton via Phabricator via cfe-commits
martong added a reviewer: a_sidorin.
martong added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D49223



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


[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-07-26 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: klimek, aprantl, pcc, sbenza, Prazek, dblaikie, 
balazske, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

Add matchSubtree, so we can traverse on a subtree rooted on a specific node.

Currently, we can match **one** node against a matcher, but we will not traverse
into the children (this is MatchFinder::match).
Or we can traverse through the whole tree rooted at the TUDecl (this
is MatchFinder::matchAST).
Note, findAll may provide an alternative, but that will traverse throught the
whole AST, and that has some weaknesses:
https://bugs.llvm.org/show_bug.cgi?id=38318


Repository:
  rC Clang

https://reviews.llvm.org/D49840

Files:
  include/clang/ASTMatchers/ASTMatchFinder.h
  lib/ASTMatchers/ASTMatchFinder.cpp
  unittests/ASTMatchers/ASTMatchersInternalTest.cpp

Index: unittests/ASTMatchers/ASTMatchersInternalTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersInternalTest.cpp
+++ unittests/ASTMatchers/ASTMatchersInternalTest.cpp
@@ -236,5 +236,53 @@
 
 #endif // _WIN32
 
+
+TEST(Matcher, matchSubtree) {
+
+  std::unique_ptr AST =
+  clang::tooling::buildASTFromCode(
+  R"(
+  template 
+  struct X {
+void f() {}
+void g() {}
+  };
+  void foo() {
+  X xc;
+  xc.f();
+  X xi;
+  }
+  )");
+  ASSERT_TRUE(AST.get());
+
+  // To get the "f" FunctionDecl inside the instantiation ...
+  auto FullPattern =
+  functionDecl(hasName("f")
+  // ... we must specify the parent.
+  ,hasParent(classTemplateSpecializationDecl()));
+  auto *FunD = selectFirst(
+  "dontcare", match(FullPattern.bind("dontcare"), AST->getASTContext()));
+
+
+  auto *SpecD = selectFirst(
+  "dontcare", match(classTemplateSpecializationDecl().bind("dontcare"),
+AST->getASTContext()));
+  MatchFinder Finder;
+  struct TestCallback : MatchFinder::MatchCallback {
+FunctionDecl *Node = nullptr;
+void run(const MatchFinder::MatchResult &Result) override {
+  Node =
+  const_cast(Result.Nodes.getNodeAs(""));
+}
+  };
+  TestCallback Cb;
+
+  // With matchSubtree we can traverse through the given instantiation.
+  auto SimplePattern = functionDecl(hasName("f"));
+  Finder.addMatcher(SimplePattern.bind(""), &Cb);
+  Finder.matchSubtree(*SpecD, AST->getASTContext());
+  EXPECT_EQ(FunD, Cb.Node);
+}
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: lib/ASTMatchers/ASTMatchFinder.cpp
===
--- lib/ASTMatchers/ASTMatchFinder.cpp
+++ lib/ASTMatchers/ASTMatchFinder.cpp
@@ -1015,6 +1015,32 @@
   Visitor.match(Node);
 }
 
+void MatchFinder::matchSubtree(const clang::ast_type_traits::DynTypedNode &Node,
+   ASTContext &Context) {
+  internal::MatchASTVisitor Visitor(&Matchers, Options);
+  Visitor.set_active_ast_context(&Context);
+  // FIXME: Improve this with a switch or a visitor pattern.
+  if (auto *N = Node.get()) {
+if (dyn_cast(N))
+  Visitor.onStartOfTranslationUnit();
+Visitor.TraverseDecl(const_cast(N));
+if (dyn_cast(N))
+  Visitor.onEndOfTranslationUnit();
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseStmt(const_cast(N));
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseType(*N);
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseTypeLoc(*N);
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseNestedNameSpecifier(const_cast(N));
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseNestedNameSpecifierLoc(*N);
+  } else if (auto *N = Node.get()) {
+Visitor.TraverseConstructorInitializer(const_cast(N));
+  }
+}
+
 void MatchFinder::matchAST(ASTContext &Context) {
   internal::MatchASTVisitor Visitor(&Matchers, Options);
   Visitor.set_active_ast_context(&Context);
Index: include/clang/ASTMatchers/ASTMatchFinder.h
===
--- include/clang/ASTMatchers/ASTMatchFinder.h
+++ include/clang/ASTMatchers/ASTMatchFinder.h
@@ -189,6 +189,15 @@
  ASTContext &Context);
   /// @}
 
+  /// \brief Finds all matches in the given subtree rooted at \p Node
+  /// @{
+  template  void matchSubtree(const T &Node, ASTContext &Context) {
+matchSubtree(clang::ast_type_traits::DynTypedNode::create(Node), Context);
+  }
+  void matchSubtree(const clang::ast_type_traits::DynTypedNode &Node,
+ASTContext &Context);
+  /// @}
+
   /// Finds all matches in the given AST.
   void matchAST(ASTContext &Context);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-07-26 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> Usually we use match(anyOf(node), hasDescendant(node)). Or did I 
> misunderstand what you want?

My understanding is that, the free function template `match` uses 
`MatchFinder::matchAST`, which will start the traverse from the 
TranslationUnitDecl.
And there is no option to pick a specific node and specify that as the root of 
the traverse. I'd like an option to be able to start the traverse from a 
specific node, if it makes sense.


Repository:
  rC Clang

https://reviews.llvm.org/D49840



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


[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-07-27 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> MatchFinder::match allows you to match a node. Wrapping your matcher code 
> with:
>  auto m = ;
>  ast_matchers::match(anyOf(m, hashDescendant(m)), node, context);

Okay, I understand and accept that.
However, I consider that a different level of abstraction. 
`ast_matchers::match` uses `internal::CollectMatchesCallback` in its 
implementation. If I already have my own custom MatchCallback implemented then 
there is no way to achieve the desired behavior.

For example, in `ASTImporter` tests we use the following customized callback 
class:

  enum class DeclMatcherKind { First, Last };
  
  // Matcher class to retrieve the first/last matched node under a given AST.
  template 
  class DeclMatcher : public MatchFinder::MatchCallback {
NodeType *Node = nullptr;
void run(const MatchFinder::MatchResult &Result) override {
  if ((MatcherKind == DeclMatcherKind::First && Node == nullptr) ||
  MatcherKind == DeclMatcherKind::Last) {
Node = const_cast(Result.Nodes.getNodeAs(""));
  }
}
  public:
// Returns the first/last matched node under the tree rooted in `D`.
template 
NodeType *match(const Decl *D, const MatcherType &AMatcher) {
  MatchFinder Finder;
  Finder.addMatcher(AMatcher.bind(""), this);
  Finder.matchAST(D->getASTContext());
  assert(Node);
  return Node;
}
  };
  template 
  using LastDeclMatcher = DeclMatcher;
  template 
  using FirstDeclMatcher = DeclMatcher;

And this is how we use it in the tests:

  Decl *FromTU = getTuDecl("void f(); void f(); void f();", Lang_CXX);
  auto Pattern = functionDecl(hasName("f"));
  auto *D0 = FirstDeclMatcher().match(FromTU, Pattern);
  auto *D2 = LastDeclMatcher().match(FromTU, Pattern);

At this point we would like to extend this `DeclMatcher` to be able to match a 
subtree, and be able to start the traverse from a specific `Decl`, something 
like this :

  auto *DV = FirsDeclMatcher().match(D2, SomeOtherPattern);

Currently, I don't see how we could do this extension without the proposed 
`matchSubtree`. 
(Perhaps, we could refactor our `DeclMatcher` to not use a customized 
MatchCallback, rather use `ast_matchers::match`, but that sounds like a bad 
workaround to me.)

Hope this makes the the goal of this patch cleaner.


Repository:
  rC Clang

https://reviews.llvm.org/D49840



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


[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-07-27 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> Finder.match also has an overload that takes the node. Can you wrap "Pattern" 
> above in the anyOf(hasDescendant(...), ...) and match on the node instead of 
> the full AST?

Ok, I changed and wrapped the pattern:

  template 
  NodeType *match(const Decl *D, const MatcherType &AMatcher) {
MatchFinder Finder;
auto WrappedMatcher = anyOf(AMatcher.bind(""), 
hasDescendant(AMatcher.bind("")));
Finder.addMatcher(WrappedMatcher, this);
// ...
  }

But this results in an ambigous call of `addMatcher` because the compiler 
cannot choose the appropriate overload:

  ../../git/llvm/tools/clang/unittests/AST/DeclMatcher.h:36:12: error: call to 
member function 'addMatcher' is ambiguous
  Finder.addMatcher(WrappedMatcher, this);
  ~~~^~
  ...
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:149:8: 
note: candidate function
void addMatcher(const DeclarationMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:151:8: 
note: candidate function
void addMatcher(const TypeMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:153:8: 
note: candidate function
void addMatcher(const StatementMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:155:8: 
note: candidate function
void addMatcher(const NestedNameSpecifierMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:157:8: 
note: candidate function
void addMatcher(const NestedNameSpecifierLocMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:159:8: 
note: candidate function
void addMatcher(const TypeLocMatcher &NodeMatch,
 ^
  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:161:8: 
note: candidate function
void addMatcher(const CXXCtorInitializerMatcher &NodeMatch,
 ^

This seems quite logical to me, since `anyOf` wraps many matchers, which could 
refer to Nodes with different type.
So, I went on and specified which overload to call (note, from this point this 
is getting hacky and would not consider such an API easy to use):

  And then I gave up.
template 
NodeType *match(const Decl *D, const MatcherType &AMatcher) {
  MatchFinder Finder;
  auto WrappedMatcher = anyOf(AMatcher.bind(""), 
hasDescendant(AMatcher.bind("")));
  auto PtrToMemFun = static_cast(&MatchFinder::addMatcher);
  (Finder.*PtrToMemFun)(WrappedMatcher, this);

This time it turned out that we can't just add a matcker like this because 
`getMatchers` in `anyOf` would call the private ctor of `Matcher`:

  
../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1339:13:
 error: calling a private constructor of class 
'clang::ast_matchers::internal::Matcher'
  return {Matcher(std::get(Params))...};
  ^
  
../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1331:16:
 note: in instantiation of function template specialization 
'clang::ast_matchers::internal::VariadicOperatorMatcher,
 
clang::ast_matchers::internal::ArgumentAdaptingMatcherFunc, 
clang::ast_matchers::internal::TypeList >::Adaptor >::getMatchers' 
requested here
 getMatchers(llvm::index_sequence_for()))
 ^

As an alternative, I tried to call addDynamicMatcher, which resulted in a 
conversion error:

  template 
  NodeType *match(const Decl *D, const MatcherType &AMatcher) {
MatchFinder Finder;
auto WrappedMatcher = anyOf(AMatcher.bind(""), 
hasDescendant(AMatcher.bind("")));
Finder.addDynamicMatcher(WrappedMatcher, this);

  ../../git/llvm/tools/clang/unittests/AST/DeclMatcher.h:40:30: error: no 
viable conversion from 
'clang::ast_matchers::internal::VariadicOperatorMatcher,
 
clang::ast_matchers::internal::ArgumentAdaptingMatcherFunc, 
clang::ast_matchers::internal::TypeList >::Adaptor >' to 'const internal::DynTypedMatcher'
  Finder.addDynamicMatcher(WrappedMatcher, this);
   ^~

Perhaps I am doing something wrong because I really could not find a way to add 
the wrapped matcher.
These results motivated me to double check your original idea of using 
`ast_matchers::match`:

  Decl *FromTU = getTuDecl("void f();", Lang_CXX);
  auto Pattern = functionDecl(hasName("f"));
  match(anyOf(Pattern, hasDescendant(Pattern)), FromTU, 
FromTU->getASTContext());

This resulted an ambigous call again, but this time the compiler could not 
resolve the call inside `ast_matchers::match`:

  ../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchFinder.h:301:10: 
error: call to member function 'addMatcher' is ambiguous
Finder.addMatcher(Matcher, &Callback);
~~~^~


Repository:
  rC Clang

https://reviews.llvm.org/D49840



___
cf

[PATCH] D49840: [AST] Add MatchFinder::matchSubtree

2018-07-27 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

> If you know the node is a decl, wrapping it in decl() should be enough.
>  Does this work?
>  auto WrappedMatcher = decl(anyOf(...));

Unfortunately this gives again the private ctor error (the same error when I 
called explicitly the overload with the `DeclarationMatcher`):

  
../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1339:13:
 error: calling a private constructor of class 
'clang::ast_matchers::internal::Matcher'
  return {Matcher(std::get(Params))...};
  ^
  
../../git/llvm/tools/clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1331:16:
 note: in instantiation of function template specialization 
'clang::ast_matchers::internal::VariadicOperatorMatcher,
 
clang::ast_matchers::internal::ArgumentAdaptingMatcherFunc, 
clang::ast_matchers::internal::TypeList >::Adaptor >::getMatchers' 
requested here
 getMatchers(llvm::index_sequence_for()))
 ^
  ../../git/llvm/tools/clang/unittests/AST/DeclMatcher.h:35:32: note: in 
instantiation of function template specialization 
'clang::ast_matchers::internal::VariadicOperatorMatcher,
 
clang::ast_matchers::internal::ArgumentAdaptingMatcherFunc, 
clang::ast_matchers::internal::TypeList >::Adaptor >::operator Matcher' 
requested here
  auto WrappedMatcher = decl(anyOf(AMatcher.bind(""), 
hasDescendant(AMatcher.bind("";
 ^

Even if it would work then how could I support nodes other than decls (e.g. 
stmt())?


Repository:
  rC Clang

https://reviews.llvm.org/D49840



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


[PATCH] D49223: [AST] Check described template at structural equivalence check.

2018-07-27 Thread 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.


Repository:
  rC Clang

https://reviews.llvm.org/D49223



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


[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

2018-02-10 Thread Gabor Marton via Phabricator via cfe-commits
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:2858
+
+  // Templated declarations should never appear in the enclosing DeclContext.
+  if (!D->getDescribedVarTemplate())

In case of class templates, the explicit instantiation is the member of the 
DeclContext. It does not belong to the DeclContext only in case of implicit 
instantiations.
I suppose the same is true for template variables as well.
In our code base we fixed it by checking on the TSK_ kind, see 
https://github.com/Ericsson/clang/pull/270/files.

@xazax.hun perhaps we should open source some of our fixes as well?



Comment at: lib/AST/ASTImporter.cpp:4455
 D2->setLexicalDeclContext(LexicalDC);
 LexicalDC->addDeclInternal(D2);
+

This is related to my other comment, perhaps we should not add `D2` to the 
DeclContext unconditionnally. I think only implicit instantiations should be 
added. See
https://github.com/Ericsson/clang/pull/270/files


Repository:
  rC Clang

https://reviews.llvm.org/D43012



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


[PATCH] D43012: [ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl

2018-02-10 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Just ignore my previous comments, the issue with explicit instantiations could 
be fixed in a separate independent patch. All is good.


Repository:
  rC Clang

https://reviews.llvm.org/D43012



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


[PATCH] D43967: Add test helper Fixture

2018-03-01 Thread Gabor Marton via Phabricator via cfe-commits
martong created this revision.
martong added reviewers: a.sidorin, xazax.hun, szepet.
Herald added subscribers: cfe-commits, dkrupp, rnkovacs.

[ASTImporter] Add a helper test Fixture, so we can add tests which may
check internal attributes of AST nodes. Also it makes possible to import from
several "From" contexts.


Repository:
  rC Clang

https://reviews.llvm.org/D43967

Files:
  unittests/AST/ASTImporterTest.cpp
  unittests/AST/DeclMatcher.h

Index: unittests/AST/DeclMatcher.h
===
--- /dev/null
+++ unittests/AST/DeclMatcher.h
@@ -0,0 +1,68 @@
+//===- unittest/AST/DeclMatcher.h - AST unit test support ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_UNITTESTS_AST_DECLMATCHER_H
+#define LLVM_CLANG_UNITTESTS_AST_DECLMATCHER_H
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace ast_matchers {
+
+enum class DeclMatcherKind { First, Last };
+
+// Matcher class to retrieve the first/last matched node under a given AST.
+template 
+class DeclMatcher : public MatchFinder::MatchCallback {
+  NodeType *Node = nullptr;
+  void run(const MatchFinder::MatchResult &Result) override {
+if ((MatcherKind == DeclMatcherKind::First && Node == nullptr) ||
+MatcherKind == DeclMatcherKind::Last) {
+  Node = const_cast(Result.Nodes.getNodeAs(""));
+}
+  }
+public:
+  // Returns the first/last matched node under the tree rooted in `D`.
+  template 
+  NodeType *match(const Decl *D, const MatcherType &AMatcher) {
+MatchFinder Finder;
+Finder.addMatcher(AMatcher.bind(""), this);
+Finder.matchAST(D->getASTContext());
+assert(Node);
+return Node;
+  }
+};
+template 
+using LastDeclMatcher = DeclMatcher;
+template 
+using FirstDeclMatcher = DeclMatcher;
+
+template 
+class DeclCounter : public MatchFinder::MatchCallback {
+  unsigned count = 0;
+  void run(const MatchFinder::MatchResult &Result) override {
+  if(Result.Nodes.getNodeAs("")) {
+++count;
+  }
+  }
+public:
+  // Returns the number of matched nodes under the tree rooted in `D`.
+  template 
+  unsigned match(const Decl *D, const MatcherType &AMatcher) {
+MatchFinder Finder;
+Finder.addMatcher(AMatcher.bind(""), this);
+Finder.matchAST(D->getASTContext());
+return count;
+  }
+};
+
+} // end namespace ast_matchers
+} // end namespace clang
+
+#endif
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -17,6 +17,8 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
+
+#include "DeclMatcher.h"
 #include "gtest/gtest.h"
 
 namespace clang {
@@ -29,7 +31,7 @@
   return Lang == Lang_CXX || Lang == Lang_CXX11;
 }
 
-static RunOptions getRunOptionsForLanguage(Language Lang) {
+static ArgVector getBasicRunOptionsForLanguage(Language Lang) {
   ArgVector BasicArgs;
   // Test with basic arguments.
   switch (Lang) {
@@ -49,6 +51,11 @@
   case Lang_OBJCXX:
 llvm_unreachable("Not implemented yet!");
   }
+  return BasicArgs;
+}
+
+static RunOptions getRunOptionsForLanguage(Language Lang) {
+  ArgVector BasicArgs = getBasicRunOptionsForLanguage(Lang);
 
   // For C++, test with "-fdelayed-template-parsing" enabled to handle MSVC
   // default behaviour.
@@ -133,6 +140,146 @@
  Verifier, AMatcher));
 }
 
+class Fixture : public ::testing::Test {
+
+  const char *const InputFileName = "input.cc";
+  const char *const OutputFileName = "output.cc";
+
+  //Buffers for the contexts, must live in test scope
+  std::string ToCode;
+
+  struct TU {
+std::string Code;
+std::string FileName;
+std::unique_ptr Unit;
+TranslationUnitDecl *TUDecl = nullptr;
+TU(StringRef Code, StringRef FileName, ArgVector Args)
+: Code(Code), FileName(FileName),
+  Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
+ this->FileName)),
+  TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {}
+  };
+  // We may have several From context and related translation units.
+  std::list FromTUs;
+
+  // Creates a virtual file and assigns that to the To context.  If the file
+  // already exists then the file will not be created again as a duplicate.
+  void createVirtualFile(StringRef FileName, const std::string &Code) {
+assert(ToAST);
+ASTContext &ToCtx = ToAST->getASTContext();
+vfs::OverlayFileSystem *OFS = static_cast(
+ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+vfs::InMemoryFileSystem *MFS =
+st

[PATCH] D43967: Add test helper Fixture

2018-03-01 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

We add several test cases here, some of them are disabled.
We plan to pass the disabled tests in different patches.


Repository:
  rC Clang

https://reviews.llvm.org/D43967



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


  1   2   3   4   5   6   7   8   9   10   >