[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, &StackTemplateArgs, /*RelativeToPrimary*/ true,
         /*Pattern*/ nullptr,
+        /*ForConceptInstantiation*/ true);
----------------



================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2879-2881
       Template, /*InnerMost*/ NeedsReplacement ? nullptr : &DeducedTAL,
       /*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<DeclContext>(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<VarTemplateSpecializationDecl>(D);
-    if (Spec && !Spec->isClassScopeExplicitSpecialization()) {
-      // We're done when we hit an explicit specialization.
-      if (Spec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-          !isa<VarTemplatePartialSpecializationDecl>(Spec))
-        return Result;
-
-      Result.addOuterTemplateArguments(&Spec->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<VarTemplateDecl*,
-                         VarTemplatePartialSpecializationDecl*> Specialized
-                             = Spec->getSpecializedTemplateOrPartial();
-      if (VarTemplatePartialSpecializationDecl *Partial =
-              Specialized.dyn_cast<VarTemplatePartialSpecializationDecl *>()) {
-        if (Partial->isMemberSpecialization())
-          return Result;
-      } else {
-        VarTemplateDecl *Tmpl = Specialized.get<VarTemplateDecl *>();
-        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<TemplateTemplateParmDecl>(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<ClassTemplateSpecializationDecl>(Ctx);
-    if (Spec && !Spec->isClassScopeExplicitSpecialization()) {
-      // We're done when we hit an explicit specialization.
-      if (Spec->getSpecializationKind() == TSK_ExplicitSpecialization &&
-          !isa<ClassTemplatePartialSpecializationDecl>(Spec))
-        break;
-
-      Result.addOuterTemplateArguments(&Spec->getTemplateInstantiationArgs());
-
-      // If this class template specialization was instantiated from a
-      // specialized member that is a class template, we're done.
-      assert(Spec->getSpecializedTemplate() && "No class template?");
-      if (Spec->getSpecializedTemplate()->isMemberSpecialization())
-        break;
-    }
-    // Add template arguments from a function template specialization.
-    else if (const auto *Function = dyn_cast<FunctionDecl>(Ctx)) {
-      if (!RelativeToPrimary &&
-          Function->getTemplateSpecializationKindForInstantiation() ==
-              TSK_ExplicitSpecialization)
-        break;
-
-      if (!RelativeToPrimary && Function->getTemplateSpecializationKind() ==
-                                    TSK_ExplicitSpecialization) {
-        // This is an implicit instantiation of an explicit specialization. We
-        // don't get any template arguments from this function but might get
-        // some from an enclosing template.
-      } else if (const TemplateArgumentList *TemplateArgs
-            = Function->getTemplateSpecializationArgs()) {
-        // Add the template arguments for this specialization.
-        Result.addOuterTemplateArguments(TemplateArgs);
-
-        // If this function was instantiated from a specialized member that is
-        // a function template, we're done.
-        assert(Function->getPrimaryTemplate() && "No function template?");
-        if (Function->getPrimaryTemplate()->isMemberSpecialization())
-          break;
-
-        // If this function is a generic lambda specialization, we are done.
-        if (!LookBeyondLambda &&
-            isGenericLambdaCallOperatorOrStaticInvokerSpecialization(Function))
-          break;
-
-      } else if (Function->getDescribedFunctionTemplate()) {
-        assert((IncludeContainingStructArgs ||
-                Result.getNumSubstitutedLevels() == 0) &&
-               "Outer template not instantiated?");
-      }
-
-      // If this is a friend declaration and it declares an entity at
-      // namespace scope, take arguments from its lexical parent
-      // instead of its semantic parent, unless of course the pattern we're
-      // instantiating actually comes from the file's context!
-      if (Function->getFriendObjectKind() &&
-          Function->getNonTransparentDeclContext()->isFileContext() &&
-          (!Pattern || !Pattern->getLexicalDeclContext()->isFileContext())) {
-        Ctx = Function->getLexicalDeclContext();
-        RelativeToPrimary = false;
-        continue;
-      }
-    } else if (const auto *Rec = dyn_cast<CXXRecordDecl>(Ctx)) {
-      if (ClassTemplateDecl *ClassTemplate = Rec->getDescribedClassTemplate()) {
-        assert((IncludeContainingStructArgs ||
-                Result.getNumSubstitutedLevels() == 0) &&
-               "Outer template not instantiated?");
-        if (ClassTemplate->isMemberSpecialization())
-          break;
-        if (IncludeContainingStructArgs) {
-          QualType RecordType = Context.getTypeDeclType(Rec);
-          QualType Injected = cast<InjectedClassNameType>(RecordType)
-                                  ->getInjectedSpecializationType();
-          const auto *InjectedType = cast<TemplateSpecializationType>(Injected);
-          Result.addOuterTemplateArguments(InjectedType->template_arguments());
+  const Decl *CurDecl = ND;
+
----------------
Should we `assert` on `ND`?


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:290
+      return Result;
+    if (R.ChangeRelativeToPrimary)
+      RelativeToPrimary = false;
----------------
This reads weird, my instant is that this should flip to the opposite value when this is `true`. Maybe an enum with names like `RelativeToPrimayOff` or something more explicit like that.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:48
+  const Decl *NextDecl = nullptr;
+  bool ChangeRelativeToPrimary = true;
+  static Response Done() {
----------------
Maybe put both `bool` fields back to back.


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

https://reviews.llvm.org/D134874