[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255-258
+/// \param ForConstraintInstantiation when collecting arguments,
+/// ForConstraintInstantiation indicates we should continue looking when
+/// encountering a lambda generic call operator, and continue looking for
+/// arguments on an enclosing class template.

erichkeane wrote:
> tahonermann wrote:
> > More a question than a review comment: why is `ForConstraintInstantiation` 
> > needed? The behavior it enables seems to me like the behavior that would 
> > always be desired. When would template arguments for such enclosing 
> > templates not be wanted?
> Apparently we DONT always want the full depth.  It is the intent to only get 
> as far back as 'needed' in some cases, and ForConstraintInstantiation causes 
> us to go 'all the way to the TU' as far as I can tell.  TBH, I'm not super 
> clear on it.  I tried at one point just modifying it, and it broke a ton of 
> tests.
Ok, thanks. My guess is that implies bugs elsewhere but that isn't a concern 
for this change!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG939a3d22e21f: [Concepts] Fix Concepts on generic lambda in a 
VarTemplateSpecDecl (authored by erichkeane).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D134874?vs=464360=464778#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134874

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts-lambda.cpp

Index: clang/test/SemaTemplate/concepts-lambda.cpp
===
--- clang/test/SemaTemplate/concepts-lambda.cpp
+++ clang/test/SemaTemplate/concepts-lambda.cpp
@@ -13,3 +13,45 @@
   f();
   };
 }
+
+namespace GH57945_2 {
+  template
+concept c = true;
+
+  template
+auto f = [](auto... args) requires c  {
+};
+
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)
+  {};
+
+  void g() {
+  f();
+  f2(5.0);
+  }
+}
+
+namespace GH57958 {
+  template concept C = true;
+  template constexpr bool v = [](C auto) { return true; }(0);
+  int _ = v<0>;
+}
+namespace GH57958_2 {
+  template concept C = true;
+  template constexpr bool v = [](C auto...) { return true; }(0);
+  int _ = v<0>;
+}
+
+namespace GH57971 {
+  template
+concept any = true;
+
+  template
+auto f = [](any auto) {
+};
+
+  using function_ptr = void(*)(int);
+  function_ptr ptr = f;
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -40,13 +40,207 @@
 // Template Instantiation Support
 //===--===/
 
+namespace {
+namespace TemplateInstArgsHelpers {
+struct Response {
+  const Decl *NextDecl = nullptr;
+  bool IsDone = false;
+  bool ClearRelativeToPrimary = true;
+  static Response Done() {
+Response R;
+R.IsDone = true;
+return R;
+  }
+  static Response ChangeDecl(const Decl *ND) {
+Response R;
+R.NextDecl = ND;
+return R;
+  }
+  static Response ChangeDecl(const DeclContext *Ctx) {
+Response R;
+R.NextDecl = Decl::castFromDeclContext(Ctx);
+return R;
+  }
+
+  static Response UseNextDecl(const Decl *CurDecl) {
+return ChangeDecl(CurDecl->getDeclContext());
+  }
+
+  static Response DontClearRelativeToPrimaryNextDecl(const Decl *CurDecl) {
+Response R = Response::UseNextDecl(CurDecl);
+R.ClearRelativeToPrimary = false;
+return R;
+  }
+};
+// Add template arguments from a variable template instantiation.
+Response
+HandleVarTemplateSpec(const VarTemplateSpecializationDecl *VarTemplSpec,
+  MultiLevelTemplateArgumentList ) {
+  // For a class-scope explicit specialization, there are no template arguments
+  // at this level, but there may be enclosing template arguments.
+  if (VarTemplSpec->isClassScopeExplicitSpecialization())
+return Response::DontClearRelativeToPrimaryNextDecl(VarTemplSpec);
+
+  // We're done when we hit an explicit specialization.
+  if (VarTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
+  !isa(VarTemplSpec))
+return Response::Done();
+
+  Result.addOuterTemplateArguments(
+  >getTemplateInstantiationArgs());
+
+  // If this variable template specialization was instantiated from a
+  // specialized member that is a variable template, we're done.
+  assert(VarTemplSpec->getSpecializedTemplate() && "No variable template?");
+  llvm::PointerUnion
+  Specialized = VarTemplSpec->getSpecializedTemplateOrPartial();
+  if (VarTemplatePartialSpecializationDecl *Partial =
+  Specialized.dyn_cast()) {
+if (Partial->isMemberSpecialization())
+  return Response::Done();
+  } else {
+VarTemplateDecl *Tmpl = Specialized.get();
+if (Tmpl->isMemberSpecialization())
+  return Response::Done();
+  }
+  return Response::DontClearRelativeToPrimaryNextDecl(VarTemplSpec);
+}
+
+// If we have a template template parameter with translation unit context,
+// then we're performing substitution into a default template argument of
+// this template template parameter before we've constructed the template
+// that will own this template template parameter. In this case, we
+// use empty template parameter lists for all of the outer templates
+// to avoid performing any substitutions.
+Response
+HandleDefaultTempArgIntoTempTempParam(const TemplateTemplateParmDecl *TTP,
+  MultiLevelTemplateArgumentList ) {
+  for (unsigned I = 

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255-258
+/// \param ForConstraintInstantiation when collecting arguments,
+/// ForConstraintInstantiation indicates we should continue looking when
+/// encountering a lambda generic call operator, and continue looking for
+/// arguments on an enclosing class template.

tahonermann wrote:
> More a question than a review comment: why is `ForConstraintInstantiation` 
> needed? The behavior it enables seems to me like the behavior that would 
> always be desired. When would template arguments for such enclosing templates 
> not be wanted?
Apparently we DONT always want the full depth.  It is the intent to only get as 
far back as 'needed' in some cases, and ForConstraintInstantiation causes us to 
go 'all the way to the TU' as far as I can tell.  TBH, I'm not super clear on 
it.  I tried at one point just modifying it, and it broke a ton of tests.


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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Tom Honermann via Phabricator via cfe-commits]

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

I think this looks good. I took more time to compare with the current code and 
it looks to me like behavioral consistency is maintained where desired.

I added one comments requesting a change to a comment (to be made when 
committing the changes) and another comment with a question asked to improve my 
own understanding (that might also indicate that some additional comments would 
be helpful).




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:75-77
+// Add template arguments from a variable template instantiation. For a
+// class-scope explicit specialization, there are no template arguments
+// at this level, but there may be enclosing template arguments.

I think the selected portion of this comment would be better placed inside the 
function before the call to `isClassScopeExplicitSpecialization`.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255-258
+/// \param ForConstraintInstantiation when collecting arguments,
+/// ForConstraintInstantiation indicates we should continue looking when
+/// encountering a lambda generic call operator, and continue looking for
+/// arguments on an enclosing class template.

More a question than a review comment: why is `ForConstraintInstantiation` 
needed? The behavior it enables seems to me like the behavior that would always 
be desired. When would template arguments for such enclosing templates not be 
wanted?


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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Johel Ernesto Guerrero Peña via Phabricator via cfe-commits]

JohelEGP added a comment.

I can confirm that, with this patch, Clang successfully compiles my actual code 
base.


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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 464360.
erichkeane marked 4 inline comments as done.
erichkeane added a comment.

Do all the things Tom requested.


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

https://reviews.llvm.org/D134874

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts-lambda.cpp

Index: clang/test/SemaTemplate/concepts-lambda.cpp
===
--- clang/test/SemaTemplate/concepts-lambda.cpp
+++ clang/test/SemaTemplate/concepts-lambda.cpp
@@ -13,3 +13,45 @@
   f();
   };
 }
+
+namespace GH57945_2 {
+  template
+concept c = true;
+
+  template
+auto f = [](auto... args) requires c  {
+};
+
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)
+  {};
+
+  void g() {
+  f();
+  f2(5.0);
+  }
+}
+
+namespace GH57958 {
+  template concept C = true;
+  template constexpr bool v = [](C auto) { return true; }(0);
+  int _ = v<0>;
+}
+namespace GH57958_2 {
+  template concept C = true;
+  template constexpr bool v = [](C auto...) { return true; }(0);
+  int _ = v<0>;
+}
+
+namespace GH57971 {
+  template
+concept any = true;
+
+  template
+auto f = [](any auto) {
+};
+
+  using function_ptr = void(*)(int);
+  function_ptr ptr = f;
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -40,13 +40,207 @@
 // Template Instantiation Support
 //===--===/
 
+namespace {
+namespace TemplateInstArgsHelpers {
+struct Response {
+  const Decl *NextDecl = nullptr;
+  bool IsDone = false;
+  bool ClearRelativeToPrimary = true;
+  static Response Done() {
+Response R;
+R.IsDone = true;
+return R;
+  }
+  static Response ChangeDecl(const Decl *ND) {
+Response R;
+R.NextDecl = ND;
+return R;
+  }
+  static Response ChangeDecl(const DeclContext *Ctx) {
+Response R;
+R.NextDecl = Decl::castFromDeclContext(Ctx);
+return R;
+  }
+
+  static Response UseNextDecl(const Decl *CurDecl) {
+return ChangeDecl(CurDecl->getDeclContext());
+  }
+
+  static Response DontClearRelativeToPrimaryNextDecl(const Decl *CurDecl) {
+Response R = Response::UseNextDecl(CurDecl);
+R.ClearRelativeToPrimary = false;
+return R;
+  }
+};
+// Add template arguments from a variable template instantiation. For a
+// class-scope explicit specialization, there are no template arguments
+// at this level, but there may be enclosing template arguments.
+Response
+HandleVarTemplateSpec(const VarTemplateSpecializationDecl *VarTemplSpec,
+  MultiLevelTemplateArgumentList ) {
+  if (VarTemplSpec->isClassScopeExplicitSpecialization())
+return Response::DontClearRelativeToPrimaryNextDecl(VarTemplSpec);
+
+  // We're done when we hit an explicit specialization.
+  if (VarTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
+  !isa(VarTemplSpec))
+return Response::Done();
+
+  Result.addOuterTemplateArguments(
+  >getTemplateInstantiationArgs());
+
+  // If this variable template specialization was instantiated from a
+  // specialized member that is a variable template, we're done.
+  assert(VarTemplSpec->getSpecializedTemplate() && "No variable template?");
+  llvm::PointerUnion
+  Specialized = VarTemplSpec->getSpecializedTemplateOrPartial();
+  if (VarTemplatePartialSpecializationDecl *Partial =
+  Specialized.dyn_cast()) {
+if (Partial->isMemberSpecialization())
+  return Response::Done();
+  } else {
+VarTemplateDecl *Tmpl = Specialized.get();
+if (Tmpl->isMemberSpecialization())
+  return Response::Done();
+  }
+  return Response::DontClearRelativeToPrimaryNextDecl(VarTemplSpec);
+}
+
+// If we have a template template parameter with translation unit context,
+// then we're performing substitution into a default template argument of
+// this template template parameter before we've constructed the template
+// that will own this template template parameter. In this case, we
+// use empty template parameter lists for all of the outer templates
+// to avoid performing any substitutions.
+Response
+HandleDefaultTempArgIntoTempTempParam(const TemplateTemplateParmDecl *TTP,
+  MultiLevelTemplateArgumentList ) {
+  for (unsigned I = 0, N = TTP->getDepth() + 1; I != N; ++I)
+Result.addOuterTemplateArguments(None);
+  return Response::Done();
+}
+
+// Add template arguments from a class template instantiation.
+Response
+HandleClassTemplateSpec(const ClassTemplateSpecializationDecl 

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 9 inline comments as done.
erichkeane added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:510
-   /*Pattern*/ nullptr,
-   /*LookBeyondLambda*/ true);
   if (SetupConstraintScope(FD, TemplateArgs, MLTAL, Scope))

tahonermann wrote:
> This would previously have passed `true` for `LookBeyondLambda` and `false` 
> for `IncludeContainingStruct`. The merge of both parameters to 
> `ForConstraintInstantiation` makes that no longer possible. I don't know if 
> that is a good thing or not. Perhaps this was a latent bug?
Yeah, I think it was. At the time I put the 2 parameters in, I did a "only set 
it if I find a place that it NEEDS it".  Thinking further through, I can't 
think of ANY reason these two needed to be different.

I ran this patch against all my workloads I have (like libcxx), and none 
changed behavior.



Comment at: clang/lib/Sema/SemaConcept.cpp:1067
- /*Pattern*/ nullptr,
- /*LookBeyondLambda*/ true);
 

tahonermann wrote:
> This would previously have passed `true` for `LookBeyondLambda` and `false` 
> for `IncludeContainingStruct`. The merge of both parameters to 
> `ForConstraintInstantiation` makes that no longer possible. I don't know if 
> that is a good thing or not. Perhaps this was a latent bug?
Same answer as above.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2881
-  /*RelativeToPrimary*/ true, /*Pattern*/
-  nullptr, /*LookBeyondLambda*/ true);
 

tahonermann wrote:
> This would previously have passed `true` for `LookBeyondLambda` and `false` 
> for `IncludeContainingStruct`. The merge of both parameters to 
> `ForConstraintInstantiation` makes that no longer possible. I don't know if 
> that is a good thing or not. Perhaps this was a latent bug?
same :) 



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:292-295
+if (R.NextDecl)
+  CurDecl = R.NextDecl;
+else
+  CurDecl = Decl::castFromDeclContext(CurDecl->getDeclContext());

tahonermann wrote:
> This seems a little fragile. How about rolling the `else` case into the main 
> if/else chain above so that `R.NextDecl` is always set? We could then assert 
> on it. I added an illustrative suggested edit above. This would require 
> changing a number of the handlers to set `NextDecl` where they currently 
> don't, but being explicit about the decl navigation in those cases might make 
> the flow easier to understand.
Hrm... an interesting idea.  I've got a patch that I'm cleaning up to do this 
that I think will end up handling this alright.  The only awkward is the 
`DontClearRelativeToPrimary` function.

I CONSIDERED doing a 'builder' pattern for `Response`, but that doesn't try 
very hard to statically enforce what we're hoping for here.  BUT, let me know 
what you think of what I have.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:246
 ///
-/// \param LookBeyondLambda Indicates that this collection of arguments should
-/// continue looking when it encounters a lambda generic call operator.
-///
-/// \param IncludeContainingStructArgs Indicates that this collection of
-/// arguments should include arguments for any class template that this
-/// declaration is included inside of.
+/// \param ForConstraintInstantiation Indicates that the collection of 
arguments
+/// should continue looking when encountering a lambda generic call operator,

shafik wrote:
> This read a little awkward.
> 
> Maybe something more like "When collecting arguments ForContraintInstantion 
> indicates that we should..."
Made another run at it. 


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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

The refactoring looks like a good improvement to me. I think there may be some 
opportunities to make it more robust. I did some spot checking that previous 
behavior is retained, but didn't exhaustively do such checking; it does look 
like many of the changes were mechanical bus as Shafik noted, it is a bit 
difficult to review.




Comment at: clang/lib/Sema/SemaConcept.cpp:510
-   /*Pattern*/ nullptr,
-   /*LookBeyondLambda*/ true);
   if (SetupConstraintScope(FD, TemplateArgs, MLTAL, Scope))

This would previously have passed `true` for `LookBeyondLambda` and `false` for 
`IncludeContainingStruct`. The merge of both parameters to 
`ForConstraintInstantiation` makes that no longer possible. I don't know if 
that is a good thing or not. Perhaps this was a latent bug?



Comment at: clang/lib/Sema/SemaConcept.cpp:1067
- /*Pattern*/ nullptr,
- /*LookBeyondLambda*/ true);
 

This would previously have passed `true` for `LookBeyondLambda` and `false` for 
`IncludeContainingStruct`. The merge of both parameters to 
`ForConstraintInstantiation` makes that no longer possible. I don't know if 
that is a good thing or not. Perhaps this was a latent bug?



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2881
-  /*RelativeToPrimary*/ true, /*Pattern*/
-  nullptr, /*LookBeyondLambda*/ true);
 

This would previously have passed `true` for `LookBeyondLambda` and `false` for 
`IncludeContainingStruct`. The merge of both parameters to 
`ForConstraintInstantiation` makes that no longer possible. I don't know if 
that is a good thing or not. Perhaps this was a latent bug?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:48
+  bool IsDone = false;
+  bool RelativeToPrimaryOff = true;
+  static Response Done() {

Suggestion: rename `RelativeToPrimaryOff` to `ClearRelativeToPrimary` or 
`SetRelativeToPrimaryFalse`.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:231
 ///
 /// \param D the declaration for which we are computing template instantiation
 /// arguments.





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:234
 ///
 /// \param Innermost if non-NULL, the innermost template argument list.
 ///

Tangent: When I was investigating this function a while back, I was uncertain 
what purpose `Innermost` serves and I didn't find the comment helpful. It 
appears that there is exactly one call to `getTemplateInstantiationArgs()` that 
passes a non-null value for it. That occurs in 
`Sema::CheckTemplateArgumentList()` and happens because a template is passed as 
`ND` and, while template arguments have been identified, a specialization has 
not yet been formed, so those arguments are passed for `Innermost`.

I suggest updating the comment:
  /// \param Innermost if non-NULL, specifies a template argument list for the 
template
  /// declaration passed as \p ND.

Perhaps it is worth adding an assert that, if `Innermost` is non-null, that 
`ND` corresponds to a template declaration (not a specialization).



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:242
 /// \param Pattern If non-NULL, indicates the pattern from which we will be
 /// instantiating the definition of the given declaration, \p D. This is
 /// used to determine the proper set of template instantiation arguments for





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:286
   }
-  bool IsFriend = Rec->getFriendObjectKind() ||
-  (Rec->getDescribedClassTemplate() &&
-   
Rec->getDescribedClassTemplate()->getFriendObjectKind());
-  if (IncludeContainingStructArgs && IsFriend &&
-  Rec->getNonTransparentDeclContext()->isFileContext() &&
-  (!Pattern || !Pattern->getLexicalDeclContext()->isFileContext())) {
-Ctx = Rec->getLexicalDeclContext();
-RelativeToPrimary = false;
-continue;
-  }
 }
 





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:292-295
+if (R.NextDecl)
+  CurDecl = R.NextDecl;
+else
+  CurDecl = Decl::castFromDeclContext(CurDecl->getDeclContext());

This seems a little fragile. How about rolling the `else` case into the main 
if/else chain above so that `R.NextDecl` is always set? We could then assert on 
it. I added an illustrative suggested edit above. This would require changing a 
number of the handlers to set `NextDecl` where they currently don't, but being 
explicit about the decl navigation in those cases might make the flow easier to 
understand.


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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 464238.
erichkeane marked 10 inline comments as done.
erichkeane added a comment.

Did @shafik s suggestions.  And yes, most of the work is just copy/pasted with 
slight changes to work in the new refactor, which does make it tougher to read, 
sorry about that :(


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

https://reviews.llvm.org/D134874

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts-lambda.cpp

Index: clang/test/SemaTemplate/concepts-lambda.cpp
===
--- clang/test/SemaTemplate/concepts-lambda.cpp
+++ clang/test/SemaTemplate/concepts-lambda.cpp
@@ -13,3 +13,45 @@
   f();
   };
 }
+
+namespace GH57945_2 {
+  template
+concept c = true;
+
+  template
+auto f = [](auto... args) requires c  {
+};
+
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)
+  {};
+
+  void g() {
+  f();
+  f2(5.0);
+  }
+}
+
+namespace GH57958 {
+  template concept C = true;
+  template constexpr bool v = [](C auto) { return true; }(0);
+  int _ = v<0>;
+}
+namespace GH57958_2 {
+  template concept C = true;
+  template constexpr bool v = [](C auto...) { return true; }(0);
+  int _ = v<0>;
+}
+
+namespace GH57971 {
+  template
+concept any = true;
+
+  template
+auto f = [](any auto) {
+};
+
+  using function_ptr = void(*)(int);
+  function_ptr ptr = f;
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -40,6 +40,191 @@
 // Template Instantiation Support
 //===--===/
 
+namespace {
+namespace TemplateInstArgsHelpers {
+struct Response {
+  const Decl *NextDecl = nullptr;
+  bool IsDone = false;
+  bool RelativeToPrimaryOff = true;
+  static Response Done() {
+Response R;
+R.IsDone = true;
+return R;
+  }
+  static Response ChangeDecl(const Decl *ND) {
+Response R;
+R.NextDecl = ND;
+return R;
+  }
+  static Response ChangeDecl(const DeclContext *Ctx) {
+Response R;
+R.NextDecl = Decl::castFromDeclContext(Ctx);
+return R;
+  }
+  static Response DontRelativeToPrimaryOff() {
+Response R;
+R.RelativeToPrimaryOff = false;
+return R;
+  }
+};
+// Add template arguments from a variable template instantiation. For a
+// class-scope explicit specialization, there are no template arguments
+// at this level, but there may be enclosing template arguments.
+Response
+HandleVarTemplateSpec(const VarTemplateSpecializationDecl *VarTemplSpec,
+  MultiLevelTemplateArgumentList ) {
+  if (VarTemplSpec->isClassScopeExplicitSpecialization())
+return Response::DontRelativeToPrimaryOff();
+
+  // We're done when we hit an explicit specialization.
+  if (VarTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
+  !isa(VarTemplSpec))
+return Response::Done();
+
+  Result.addOuterTemplateArguments(
+  >getTemplateInstantiationArgs());
+
+  // If this variable template specialization was instantiated from a
+  // specialized member that is a variable template, we're done.
+  assert(VarTemplSpec->getSpecializedTemplate() && "No variable template?");
+  llvm::PointerUnion
+  Specialized = VarTemplSpec->getSpecializedTemplateOrPartial();
+  if (VarTemplatePartialSpecializationDecl *Partial =
+  Specialized.dyn_cast()) {
+if (Partial->isMemberSpecialization())
+  return Response::Done();
+  } else {
+VarTemplateDecl *Tmpl = Specialized.get();
+if (Tmpl->isMemberSpecialization())
+  return Response::Done();
+  }
+  return Response::DontRelativeToPrimaryOff();
+}
+
+// If we have a template template parameter with translation unit context,
+// then we're performing substitution into a default template argument of
+// this template template parameter before we've constructed the template
+// that will own this template template parameter. In this case, we
+// use empty template parameter lists for all of the outer templates
+// to avoid performing any substitutions.
+Response
+HandleDefaultTempArgIntoTempTempParam(const TemplateTemplateParmDecl *TTP,
+  MultiLevelTemplateArgumentList ) {
+  for (unsigned I = 0, N = TTP->getDepth() + 1; I != N; ++I)
+Result.addOuterTemplateArguments(None);
+  return Response::Done();
+}
+
+// Add template arguments from a class template instantiation.
+Response
+HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,
+MultiLevelTemplateArgumentList ) {

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I made mostly minor comments, the diff is difficult to follow wrt to what has 
really changed but it mostly makes sense. I will probably need a second look.




Comment at: clang/lib/Sema/SemaConcept.cpp:508-510
   MLTAL = getTemplateInstantiationArgs(FD, nullptr, /*RelativeToPrimary*/ true,
/*Pattern*/ nullptr,
+   /*ForConstraintInstantiation*/ true);





Comment at: clang/lib/Sema/SemaConcept.cpp:584-586
   ND, nullptr, /*RelativeToPrimary*/ true,
   /*Pattern*/ nullptr,
+  /*ForConstraintInstantiation*/ true);





Comment at: clang/lib/Sema/SemaConcept.cpp:1065-1067
  /*RelativeToPrimary*/ true,
  /*Pattern*/ nullptr,
+ /*ForConstraintInstantiation*/ true);





Comment at: clang/lib/Sema/SemaTemplate.cpp:6106-6108
 Template, , /*RelativeToPrimary*/ true,
 /*Pattern*/ nullptr,
+/*ForConceptInstantiation*/ true);





Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2879-2881
   Template, /*InnerMost*/ NeedsReplacement ? nullptr : ,
   /*RelativeToPrimary*/ true, /*Pattern*/
+  nullptr, /*ForConstraintInstantiation*/ true);





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:246
 ///
-/// \param LookBeyondLambda Indicates that this collection of arguments should
-/// continue looking when it encounters a lambda generic call operator.
-///
-/// \param IncludeContainingStructArgs Indicates that this collection of
-/// arguments should include arguments for any class template that this
-/// declaration is included inside of.
+/// \param ForConstraintInstantiation Indicates that the collection of 
arguments
+/// should continue looking when encountering a lambda generic call operator,

This read a little awkward.

Maybe something more like "When collecting arguments ForContraintInstantion 
indicates that we should..."



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255
+bool ForConstraintInstantiation) {
+  //  bool IncludeContainingStructArgs) {
   // Accumulate the set of template argument lists in this structure.

Dead code?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:262
 
-  const auto *Ctx = dyn_cast(D);
-  if (!Ctx) {
-Ctx = D->getDeclContext();
-
-// Add template arguments from a variable template instantiation. For a
-// class-scope explicit specialization, there are no template arguments
-// at this level, but there may be enclosing template arguments.
-const auto *Spec = dyn_cast(D);
-if (Spec && !Spec->isClassScopeExplicitSpecialization()) {
-  // We're done when we hit an explicit specialization.
-  if (Spec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-  !isa(Spec))
-return Result;
-
-  Result.addOuterTemplateArguments(>getTemplateInstantiationArgs());
-
-  // If this variable template specialization was instantiated from a
-  // specialized member that is a variable template, we're done.
-  assert(Spec->getSpecializedTemplate() && "No variable template?");
-  llvm::PointerUnion Specialized
- = Spec->getSpecializedTemplateOrPartial();
-  if (VarTemplatePartialSpecializationDecl *Partial =
-  Specialized.dyn_cast()) {
-if (Partial->isMemberSpecialization())
-  return Result;
-  } else {
-VarTemplateDecl *Tmpl = Specialized.get();
-if (Tmpl->isMemberSpecialization())
-  return Result;
-  }
-}
-
-// If we have a template template parameter with translation unit context,
-// then we're performing substitution into a default template argument of
-// this template template parameter before we've constructed the template
-// that will own this template template parameter. In this case, we
-// use empty template parameter lists for all of the outer templates
-// to avoid performing any substitutions.
-if (Ctx->isTranslationUnit()) {
-  if (const auto *TTP = dyn_cast(D)) {
-for (unsigned I = 0, N = TTP->getDepth() + 1; I != N; ++I)
-  Result.addOuterTemplateArguments(None);
-return Result;
-  }
-}
-  }
-
-  while (!Ctx->isFileContext()) {
-// Add template arguments from a class template instantiation.
-const auto *Spec = dyn_cast(Ctx);
-if (Spec && !Spec->isClassScopeExplicitSpecialization()) {
-  // We're done when we hit an explicit specialization.
-  if (Spec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-  !isa(Spec))
-break;
-
-  

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane updated this revision to Diff 464003.
erichkeane added a comment.

Looks like that problem was fixed easy enough, so back to all tests passing :)


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

https://reviews.llvm.org/D134874

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts-lambda.cpp

Index: clang/test/SemaTemplate/concepts-lambda.cpp
===
--- clang/test/SemaTemplate/concepts-lambda.cpp
+++ clang/test/SemaTemplate/concepts-lambda.cpp
@@ -13,3 +13,45 @@
   f();
   };
 }
+
+namespace GH57945_2 {
+  template
+concept c = true;
+
+  template
+auto f = [](auto... args) requires c  {
+};
+
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)
+  {};
+
+  void g() {
+  f();
+  f2(5.0);
+  }
+}
+
+namespace GH57958 {
+  template concept C = true;
+  template constexpr bool v = [](C auto) { return true; }(0);
+  int _ = v<0>;
+}
+namespace GH57958_2 {
+  template concept C = true;
+  template constexpr bool v = [](C auto...) { return true; }(0);
+  int _ = v<0>;
+}
+
+namespace GH57971 {
+  template
+concept any = true;
+
+  template
+auto f = [](any auto) {
+};
+
+  using function_ptr = void(*)(int);
+  function_ptr ptr = f;
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -40,6 +40,191 @@
 // Template Instantiation Support
 //===--===/
 
+namespace {
+namespace TemplateInstArgsHelpers {
+struct Response {
+  bool IsDone = false;
+  const Decl *NextDecl = nullptr;
+  bool ChangeRelativeToPrimary = true;
+  static Response Done() {
+Response R;
+R.IsDone = true;
+return R;
+  }
+  static Response ChangeDecl(const Decl *ND) {
+Response R;
+R.NextDecl = ND;
+return R;
+  }
+  static Response ChangeDecl(const DeclContext *Ctx) {
+Response R;
+R.NextDecl = Decl::castFromDeclContext(Ctx);
+return R;
+  }
+  static Response DontChangeRelativeToPrimary() {
+Response R;
+R.ChangeRelativeToPrimary = false;
+return R;
+  }
+};
+// Add template arguments from a variable template instantiation. For a
+// class-scope explicit specialization, there are no template arguments
+// at this level, but there may be enclosing template arguments.
+Response
+HandleVarTemplateSpec(const VarTemplateSpecializationDecl *VarTemplSpec,
+  MultiLevelTemplateArgumentList ) {
+  if (VarTemplSpec->isClassScopeExplicitSpecialization())
+return Response::DontChangeRelativeToPrimary();
+
+  // We're done when we hit an explicit specialization.
+  if (VarTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
+  !isa(VarTemplSpec))
+return Response::Done();
+
+  Result.addOuterTemplateArguments(
+  >getTemplateInstantiationArgs());
+
+  // If this variable template specialization was instantiated from a
+  // specialized member that is a variable template, we're done.
+  assert(VarTemplSpec->getSpecializedTemplate() && "No variable template?");
+  llvm::PointerUnion
+  Specialized = VarTemplSpec->getSpecializedTemplateOrPartial();
+  if (VarTemplatePartialSpecializationDecl *Partial =
+  Specialized.dyn_cast()) {
+if (Partial->isMemberSpecialization())
+  return Response::Done();
+  } else {
+VarTemplateDecl *Tmpl = Specialized.get();
+if (Tmpl->isMemberSpecialization())
+  return Response::Done();
+  }
+  return Response::DontChangeRelativeToPrimary();
+}
+
+// If we have a template template parameter with translation unit context,
+// then we're performing substitution into a default template argument of
+// this template template parameter before we've constructed the template
+// that will own this template template parameter. In this case, we
+// use empty template parameter lists for all of the outer templates
+// to avoid performing any substitutions.
+Response
+HandleDefaultTempArgIntoTempTempParam(const TemplateTemplateParmDecl *TTP,
+  MultiLevelTemplateArgumentList ) {
+  for (unsigned I = 0, N = TTP->getDepth() + 1; I != N; ++I)
+Result.addOuterTemplateArguments(None);
+  return Response::Done();
+}
+
+// Add template arguments from a class template instantiation.
+Response
+HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,
+MultiLevelTemplateArgumentList ) {
+  if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {
+// We're done when we hit an explicit specialization.
+if 

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/test/SemaTemplate/concepts-lambda.cpp:26
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)

Huh... this f2 example seems to ICE for some reason, I could swear it worked, 
but building a different patch on top of this one showed me this fails.

Looking into it now, but it isn't critical to this patch, so if the rest looks 
good, I might remove this test case and fix it in a followup.


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

https://reviews.llvm.org/D134874

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

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

[Erich Keane via Phabricator via cfe-commits]

erichkeane created this revision.
erichkeane added reviewers: tahonermann, shafik.
Herald added a project: All.
erichkeane requested review of this revision.

As fallout of the Deferred Concept Instantiation patch (babdef27c5 
), we
got a number of reports of a regression, where we asserted when
instantiating a constraint on a generic lambda inside of a variable
template. See: https://github.com/llvm/llvm-project/issues/57958

The problem was that getTemplateInstantiationArgs function only walked
up declaration contexts, and missed that this is not necessarily the
case with a lambda (which can ALSO be in a separate context).

This patch refactors the getTemplateInstantiationArgs function in a way
that is hopefully more readable, and fixes the problem with the concepts
on a generic lambda.


https://reviews.llvm.org/D134874

Files:
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/DeclBase.cpp
  clang/lib/Sema/SemaConcept.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaTemplate/concepts-lambda.cpp

Index: clang/test/SemaTemplate/concepts-lambda.cpp
===
--- clang/test/SemaTemplate/concepts-lambda.cpp
+++ clang/test/SemaTemplate/concepts-lambda.cpp
@@ -13,3 +13,45 @@
   f();
   };
 }
+
+namespace GH57945_2 {
+  template
+concept c = true;
+
+  template
+auto f = [](auto... args) requires c  {
+};
+
+  template 
+  auto f2 = [](auto... args)
+requires (sizeof...(args) > 0)
+  {};
+
+  void g() {
+  f();
+  f2(5.0);
+  }
+}
+
+namespace GH57958 {
+  template concept C = true;
+  template constexpr bool v = [](C auto) { return true; }(0);
+  int _ = v<0>;
+}
+namespace GH57958_2 {
+  template concept C = true;
+  template constexpr bool v = [](C auto...) { return true; }(0);
+  int _ = v<0>;
+}
+
+namespace GH57971 {
+  template
+concept any = true;
+
+  template
+auto f = [](any auto) {
+};
+
+  using function_ptr = void(*)(int);
+  function_ptr ptr = f;
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -40,6 +40,191 @@
 // Template Instantiation Support
 //===--===/
 
+namespace {
+namespace TemplateInstArgsHelpers {
+struct Response {
+  bool IsDone = false;
+  const Decl *NextDecl = nullptr;
+  bool ChangeRelativeToPrimary = true;
+  static Response Done() {
+Response R;
+R.IsDone = true;
+return R;
+  }
+  static Response ChangeDecl(const Decl *ND) {
+Response R;
+R.NextDecl = ND;
+return R;
+  }
+  static Response ChangeDecl(const DeclContext *Ctx) {
+Response R;
+R.NextDecl = Decl::castFromDeclContext(Ctx);
+return R;
+  }
+  static Response DontChangeRelativeToPrimary() {
+Response R;
+R.ChangeRelativeToPrimary = false;
+return R;
+  }
+};
+// Add template arguments from a variable template instantiation. For a
+// class-scope explicit specialization, there are no template arguments
+// at this level, but there may be enclosing template arguments.
+Response
+HandleVarTemplateSpec(const VarTemplateSpecializationDecl *VarTemplSpec,
+  MultiLevelTemplateArgumentList ) {
+  if (VarTemplSpec->isClassScopeExplicitSpecialization())
+return Response::DontChangeRelativeToPrimary();
+
+  // We're done when we hit an explicit specialization.
+  if (VarTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
+  !isa(VarTemplSpec))
+return Response::Done();
+
+  Result.addOuterTemplateArguments(
+  >getTemplateInstantiationArgs());
+
+  // If this variable template specialization was instantiated from a
+  // specialized member that is a variable template, we're done.
+  assert(VarTemplSpec->getSpecializedTemplate() && "No variable template?");
+  llvm::PointerUnion
+  Specialized = VarTemplSpec->getSpecializedTemplateOrPartial();
+  if (VarTemplatePartialSpecializationDecl *Partial =
+  Specialized.dyn_cast()) {
+if (Partial->isMemberSpecialization())
+  return Response::Done();
+  } else {
+VarTemplateDecl *Tmpl = Specialized.get();
+if (Tmpl->isMemberSpecialization())
+  return Response::Done();
+  }
+  return Response::DontChangeRelativeToPrimary();
+}
+
+// If we have a template template parameter with translation unit context,
+// then we're performing substitution into a default template argument of
+// this template template parameter before we've constructed the template
+// that will own this template template parameter. In this case, we
+// use empty template parameter lists for all of the outer templates
+// to avoid performing any substitutions.