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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
