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