[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/

https://reviews.llvm.org/D134874