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
