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

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

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

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58673

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

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

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

2019-03-15 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik accepted this revision.
shafik added a comment.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-03-14 Thread Gabor Marton via Phabricator via cfe-commits 
martong marked an inline comment as done.
martong added a comment.

@shafik Ping


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-03-07 Thread Gabor Marton via Phabricator via cfe-commits 
martong marked 2 inline comments as done.
martong added inline comments.



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

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

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

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

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



Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-03-05 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added inline comments.



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

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-03-04 Thread Gabor Marton via Phabricator via cfe-commits 
martong added a comment.

Alexei, thanks for the review!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-03-04 Thread Gabor Marton via Phabricator via cfe-commits 
martong updated this revision to Diff 189131.
martong marked 2 inline comments as done.
martong added a comment.

- Fix some comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673

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

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

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

2019-03-03 Thread 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.

Hi Gabor,
Thanks for the patch! It looks good to me except some stylish nits.




Comment at: lib/AST/ASTImporter.cpp:5130
+Importer.MapImported(D, PrevDecl->getDefinition());
+// Import those those default field initializers which have been
+// instantiated in the "From" context, but not in the "To" context.

Could you please also replace "those those" to "those" in the comment below?



Comment at: lib/AST/ASTImporter.cpp:5144
+// Generally, ASTCommon.h/DeclUpdateKind enum gives a very good hint
+// what
+// else could be fused during an AST merge.

The comment is a bit broken.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58673



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

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

2019-02-26 Thread Gabor Marton via Phabricator via cfe-commits 
martong created this revision.
martong added reviewers: a_sidorin, shafik.
Herald added subscribers: cfe-commits, jdoerfert, gamesh411, Szelethus, dkrupp, 
rnkovacs.
Herald added a reviewer: a.sidorin.
Herald added a project: clang.

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


Repository:
  rC Clang

https://reviews.llvm.org/D58673

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

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