[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL332509: [Sema] Fix assertion when constructor is disabled 
with partially specialized… (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46909?vs=146944=147144#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46909

Files:
  cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
  cfe/trunk/test/SemaTemplate/partial-spec-instantiate.cpp

Index: cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
===
--- cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
+++ cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to determine
+//   whether the instantiation is to be generated using the primary
+//   template or one of the partial specializations. This is done by
+//   matching the template arguments of the class template
+//   specialization with the template argument lists of the partial
+//   specializations.
+typedef PartialSpecMatchResult MatchResult;
+SmallVector Matched;
+SmallVector PartialSpecs;
+Template->getPartialSpecializations(PartialSpecs);
+TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
+for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
+  ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
+  TemplateDeductionInfo Info(FailedCandidates.getLocation());
+  if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
+  Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
+// Store the failed-deduction information for use in diagnostics, later.
+// TODO: Actually use the failed-deduction info?
+FailedCandidates.addCandidate().set(
+DeclAccessPair::make(Template, AS_public), Partial,
+MakeDeductionFailureInfo(S.Context, Result, Info));
+(void)Result;
+  } else {
+Matched.push_back(PartialSpecMatchResult());
+Matched.back().Partial = Partial;
+Matched.back().Args = Info.take();
+  }
 }
-  }
 
-  // If we're dealing with a member template where the template parameters
-  // have been instantiated, this provides the original template parameters
-  // from which the member template's parameters were instantiated.
-
-  if (Matched.size() >= 1) {
-SmallVectorImpl::iterator Best = Matched.begin();
-if (Matched.size() == 1) {
-  //   -- If exactly one matching specialization is found, the
-  //  instantiation is generated from that specialization.
-  // We don't need to do anything for this.
-} else {
-  //   -- If more than one matching specialization is found, the
-  //  partial order rules 

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the quick review, Richard. I'll keep in mind the idea with comparing 
results of multiple lookups when I work on other partial specialization-related 
bugs.


https://reviews.llvm.org/D46909



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Looks good. It might be beneficial to do the lookup twice in this case and then 
issue a diagnostic if the results differ, but given how rarely this is 
happening, it doesn't really seem worthwhile.


https://reviews.llvm.org/D46909



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai updated this revision to Diff 146944.
vsapsai added a comment.

Potentially, VarTemplateDecl is susceptible to the same bug as
ClassTemplateSpecializationDecl. But I wasn't able to trigger it. And based on
code I've convinced myself that the mentioned early exit in
Sema::CheckVarTemplateId is equivalent to reusing available partial
specialization. So seems like no changes for VarTemplateDecl are required.


https://reviews.llvm.org/D46909

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/partial-spec-instantiate.cpp

Index: clang/test/SemaTemplate/partial-spec-instantiate.cpp
===
--- clang/test/SemaTemplate/partial-spec-instantiate.cpp
+++ clang/test/SemaTemplate/partial-spec-instantiate.cpp
@@ -55,3 +55,46 @@
   // expected-no-diagnostics
 #endif
 }
+
+// rdar://problem/39524996
+namespace rdar39524996 {
+  template 
+  struct enable_if_not_same
+  {
+typedef void type;
+  };
+  template 
+  struct enable_if_not_same;
+
+  template 
+  struct Wrapper {
+// Assertion triggered on trying to set twice the same partial specialization
+// enable_if_not_same
+template 
+Wrapper(const Wrapper& other,
+typename enable_if_not_same::type* = 0) {}
+
+explicit Wrapper(int i) {}
+  };
+
+  template 
+  struct Container {
+// It is important that the struct has implicit copy and move constructors.
+Container() : x() {}
+
+template 
+Container(const Container& other) : x(static_cast(other.x)) {}
+
+// Implicit constructors are member-wise, so the field triggers instantiation
+// of T constructors and we instantiate all of them for overloading purposes.
+T x;
+  };
+
+  void takesWrapperInContainer(const Container< Wrapper >& c);
+  void test() {
+// Type mismatch triggers initialization with conversion which requires
+// implicit constructors to be instantiated.
+Container c;
+takesWrapperInContainer(c);
+  }
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to determine
+//   whether the instantiation is to be generated using the primary
+//   template or one of the partial specializations. This is done by
+//   matching the template arguments of the class template
+//   specialization with the template argument lists of the partial
+//   specializations.
+typedef PartialSpecMatchResult MatchResult;
+SmallVector Matched;
+SmallVector PartialSpecs;
+Template->getPartialSpecializations(PartialSpecs);
+TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
+for 

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/Sema/SemaTemplate.cpp:3873-3881
   // Find the variable template specialization declaration that
   // corresponds to these arguments.
   void *InsertPos = nullptr;
   if (VarTemplateSpecializationDecl *Spec = Template->findSpecialization(
   Converted, InsertPos)) {
 checkSpecializationVisibility(TemplateNameLoc, Spec);
 // If we already have a variable template specialization, return it.

Code of interest (will refer to it later).


https://reviews.llvm.org/D46909



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

[PATCH] D46909: [Sema] Fix assertion when constructor is disabled with partially specialized template.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added reviewers: rsmith, arphaman.

The added test case was triggering assertion

> Assertion failed: 
> (!SpecializedTemplate.is() && "Already set 
> to a class template partial specialization!"), function setInstantiationOf, 
> file clang/include/clang/AST/DeclTemplate.h, line 1825.

It was happening with ClassTemplateSpecializationDecl
`enable_if_not_same`. Because this template is specialized for
equal types not to have a definition, it wasn't instantiated and its
specialization kind remained TSK_Undeclared. And because it was implicit
instantiation, we didn't mark the decl as invalid. So when we try to
find the best matching partial specialization the second time, we hit
the assertion as partial specialization is already set.

Fix by reusing stored partial specialization when available, instead of
looking for the best match every time.

rdar://problem/39524996


https://reviews.llvm.org/D46909

Files:
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/partial-spec-instantiate.cpp

Index: clang/test/SemaTemplate/partial-spec-instantiate.cpp
===
--- clang/test/SemaTemplate/partial-spec-instantiate.cpp
+++ clang/test/SemaTemplate/partial-spec-instantiate.cpp
@@ -55,3 +55,46 @@
   // expected-no-diagnostics
 #endif
 }
+
+// rdar://problem/39524996
+namespace rdar39524996 {
+  template 
+  struct enable_if_not_same
+  {
+typedef void type;
+  };
+  template 
+  struct enable_if_not_same;
+
+  template 
+  struct Wrapper {
+// Assertion triggered on trying to set twice the same partial specialization
+// enable_if_not_same
+template 
+Wrapper(const Wrapper& other,
+typename enable_if_not_same::type* = 0) {}
+
+explicit Wrapper(int i) {}
+  };
+
+  template 
+  struct Container {
+// It is important that the struct has implicit copy and move constructors.
+Container() : x() {}
+
+template 
+Container(const Container& other) : x(static_cast(other.x)) {}
+
+// Implicit constructors are member-wise, so the field triggers instantiation
+// of T constructors and we instantiate all of them for overloading purposes.
+T x;
+  };
+
+  void takesWrapperInContainer(const Container< Wrapper >& c);
+  void test() {
+// Type mismatch triggers initialization with conversion which requires
+// implicit constructors to be instantiated.
+Container c;
+takesWrapperInContainer(c);
+  }
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2398,127 +2398,137 @@
   if (Inst.isInvalid() || Inst.isAlreadyInstantiating())
 return nullptr;
 
-  ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
-  CXXRecordDecl *Pattern = nullptr;
-
-  // C++ [temp.class.spec.match]p1:
-  //   When a class template is used in a context that requires an
-  //   instantiation of the class, it is necessary to determine
-  //   whether the instantiation is to be generated using the primary
-  //   template or one of the partial specializations. This is done by
-  //   matching the template arguments of the class template
-  //   specialization with the template argument lists of the partial
-  //   specializations.
-  typedef PartialSpecMatchResult MatchResult;
-  SmallVector Matched;
-  SmallVector PartialSpecs;
-  Template->getPartialSpecializations(PartialSpecs);
-  TemplateSpecCandidateSet FailedCandidates(PointOfInstantiation);
-  for (unsigned I = 0, N = PartialSpecs.size(); I != N; ++I) {
-ClassTemplatePartialSpecializationDecl *Partial = PartialSpecs[I];
-TemplateDeductionInfo Info(FailedCandidates.getLocation());
-if (Sema::TemplateDeductionResult Result = S.DeduceTemplateArguments(
-Partial, ClassTemplateSpec->getTemplateArgs(), Info)) {
-  // Store the failed-deduction information for use in diagnostics, later.
-  // TODO: Actually use the failed-deduction info?
-  FailedCandidates.addCandidate().set(
-  DeclAccessPair::make(Template, AS_public), Partial,
-  MakeDeductionFailureInfo(S.Context, Result, Info));
-  (void)Result;
-} else {
-  Matched.push_back(PartialSpecMatchResult());
-  Matched.back().Partial = Partial;
-  Matched.back().Args = Info.take();
+  llvm::PointerUnion
+  Specialized = ClassTemplateSpec->getSpecializedTemplateOrPartial();
+  if (!Specialized.is()) {
+// Find best matching specialization.
+ClassTemplateDecl *Template = ClassTemplateSpec->getSpecializedTemplate();
+
+// C++ [temp.class.spec.match]p1:
+//   When a class template is used in a context that requires an
+//   instantiation of the class, it is necessary to