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

[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] D50451: [ASTImporter] Fix import of class templates partial specialization

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Thank you!


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

[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] D50451: [ASTImporter] Fix import of class templates partial specialization

[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

[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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin 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:
> 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?


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

[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] D50451: [ASTImporter] Fix import of class templates partial specialization

[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

[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

[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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hi Gabor,

While importing methods looks harmless, importing fields can be a breaking 
change. Do you have any test results on this?




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

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?



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);

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.



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

The result of import is unchecked here and below. Is it intentional?



Comment at: unittests/AST/ASTImporterTest.cpp:2722
+  ASSERT_FALSE(ToFun->hasBody());
+   auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+   ASSERT_TRUE(ImportedSpec);

3-space indentation.



Comment at: unittests/AST/ASTImporterTest.cpp:2763
+  ASSERT_FALSE(ToCtor->hasBody());
+   auto *ImportedSpec = Import(FromSpec, Lang_CXX11);
+  ASSERT_TRUE(ImportedSpec);

Broken indent.


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