[PATCH] D119544: Deferred Concept Instantiation Implementation

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 10 inline comments as done.
erichkeane 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
----------------
ChuanqiXu wrote:
> 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`.
Woops, I DID mess that up :)  I will switch to basically your wording.

This _IS_ generally covered by TK_NonTemplate (and was before), and I considered not adding it at all, but it is really nice to check `getTemplatedKind` to see if the value I need is going to be there.


================
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
----------------
ChuanqiXu wrote:
> > 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.
This is actually the cases where it is NOT a local lambda or function object inside a function template.  I'll give a reword a go.


================
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);
----------------
ChuanqiXu wrote:
> 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.
Yeah, thats a pretty low quality comment as well :/  I'll give it another shot that will hopefully be more understandable.  It isn't quite what you said though.


================
Comment at: clang/lib/AST/Decl.cpp:3787
+  assert(TemplateOrSpecialization.isNull() &&
+         "Member function is already a specialization");
+  TemplateOrSpecialization = FD;
----------------
ChuanqiXu wrote:
> Do I understand incorrectly? Must it be a member function?
You do not understand incorrectly :/  Copy-paste error.  I'll remove 'member'.


================
Comment at: clang/lib/AST/DeclBase.cpp:299
+       DC && !DC->isTranslationUnit() && !DC->isNamespace();
+       DC = DC->getParent())
+    if (DC->isFunctionOrMethod())
----------------
ChuanqiXu wrote:
> From the function name, I image it should be `DC = DC->getLexicalParent`. Is it incorrect?
This is 'exactly' the function above (`getParentFunctionOrMethod`), except it uses the `Lexical` context instead of the semantic context.  Would you prefer a name of 'getParentFunctionOrMethodLexically`?


================
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,
----------------
ChuanqiXu wrote:
> 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.
Changes made!


>And I am wondering if we could hit these 2 checks only...
We cannot, this applies generally, including in cases where the current function is a local lambda or function object.


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

https://reviews.llvm.org/D119544