[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