[PATCH] D119544: Deferred Concept Instantiation Implementation

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:1891
+    TK_DependentFunctionTemplateSpecialization,
+    // A Dependent function that itself is not a function.
+    TK_DependentNonTemplate
----------------
hmmm, what does this literally mean? In my understanding, it should be:

       A non template function which is in a dependent scope.

I am just wondering if this is covered by `TK_NonTemplate`.


================
Comment at: clang/include/clang/AST/Decl.h:1942
+  /// For non-templates, this value will be NULL, unless this was instantiated
+  /// as an inner-declared function in another function template, which will
+  /// cause this to have a pointer to a FunctionDecl. For function declarations
----------------
> an inner-declared function in another function template

Does this refer to local lambdas or functions in local classes of a template function **only**? If yes, I recommend to reword this. Since I understand it by the review comment instead of the comments itself.


================
Comment at: clang/include/clang/AST/Decl.h:2691-2692
 
+  /// Specify the function that this was instantiated from, despite it not,
+  /// itself being a template.
+  void setInstantiatedFromDecl(FunctionDecl *FD);
----------------
I can't read the original comment... I am not sure if it is my problem but I think it may be better to reword it.


================
Comment at: clang/include/clang/AST/DeclBase.h:909-910
 
+  /// Does the same thing as getParentFunctionOrMethod, except starts withthe
+  /// Lexical declaration context instead.
+  const DeclContext *getLexicalParentFunctionOrMethod() const;
----------------



================
Comment at: clang/lib/AST/Decl.cpp:3787
+  assert(TemplateOrSpecialization.isNull() &&
+         "Member function is already a specialization");
+  TemplateOrSpecialization = FD;
----------------
Do I understand incorrectly? Must it be a member function?


================
Comment at: clang/lib/AST/DeclBase.cpp:299
+       DC && !DC->isTranslationUnit() && !DC->isNamespace();
+       DC = DC->getParent())
+    if (DC->isFunctionOrMethod())
----------------
>From the function name, I image it should be `DC = DC->getLexicalParent`. Is it incorrect?


================
Comment at: clang/lib/Sema/SemaConcept.cpp:480-488
+  if (DeclContext *ParentFunc = FD->getParentFunctionOrMethod()) {
+    return SetupConstraintScope(cast<FunctionDecl>(ParentFunc), TemplateArgs,
+                                MLTAL, Scope);
+  } else if (DeclContext *ParentFunc = FD->getLexicalParentFunctionOrMethod()) {
+    // In the case of functions-declared-in-functions, the DeclContext is the
+    // TU, so make sure we get the LEXICAL decl context in this case.
+    return SetupConstraintScope(cast<FunctionDecl>(ParentFunc), TemplateArgs,
----------------
Don't use else-after-return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

And I am wondering if we could hit these 2 checks only if the FD is TK_DependentNonTemplate. If yes, I think we could move these two checks in the above block. In this way, the code could be simplified further.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:4705
       CheckConstraintSatisfaction(
-          NamedConcept, {NamedConcept->getConstraintExpr()}, Converted,
+          NamedConcept, {NamedConcept->getConstraintExpr()}, MLTAL,
           SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
----------------
erichkeane wrote:
> ChuanqiXu wrote:
> > erichkeane wrote:
> > > ChuanqiXu wrote:
> > > > I would feel better if we could write:
> > > > ```
> > > > CheckConstraintSatisfaction(
> > > >           NamedConcept, {NamedConcept->getConstraintExpr()}, {MLTAL},
> > > >           SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
> > > >                       TemplateArgs->getRAngleLoc()),
> > > >           Satisfaction)
> > > > ```
> > > > 
> > > > But it looks unimplementable.
> > > I'm not sure I get the suggestion?  Why would you want to put the `MultiLevelTemplateArgumentList` in curleys?  
> > I just feel like the style is more cleaner. But I found the constructor might not allow us to do so... So this one might not be a suggestion.
> Ah, you mean to pass 'converted' directly in, so:
> ```
> CheckConstraintSatisfaction(
>           NamedConcept, {NamedConcept->getConstraintExpr()}, {Converted},
>           SourceRange(SS.isSet() ? SS.getBeginLoc() : ConceptNameInfo.getLoc(),
>                       TemplateArgs->getRAngleLoc()),
>           Satisfaction)
> ```
> 
> (notice 'Converted' instead of MLTAL).  I agree with you, that WOULD be nicer, but unfortunately I think the constructor for that type was created explicitly to avoid us doing that :)
Yeah, this is what mean. I understood it is not easy/good to implement.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:2163
+  } else if (!isFriend) {
+    Function->setInstantiatedFromDecl(D);
   }
----------------
It looks better to add some comments.


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

https://reviews.llvm.org/D119544