[PATCH] D134604: [clang] Implement sugared substitution changes to infrastructure
Erich Keane via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 24 06:55:13 PDT 2022
erichkeane accepted this revision.
erichkeane added a comment.
A couple of nits, otherwise I don't have to see this again.
================
Comment at: clang/include/clang/AST/TemplateName.h:158
+ // When true the substitution will be finalized (subst node won't be placed).
+ bool getFinal() const;
+
----------------
Based on this comment, should this be 'Finalize' instead of 'Final'? Or something like "ShouldFinalize"?
================
Comment at: clang/include/clang/Sema/Template.h:104
/// Construct a single-level template argument list.
- MultiLevelTemplateArgumentList(Decl *D, ArgList Args) {
- addOuterTemplateArguments(D, Args);
+ MultiLevelTemplateArgumentList(Decl *D, ArgList Args, bool Final) {
+ addOuterTemplateArguments(D, Args, Final);
----------------
Hrmph, this additional flag likely requires rebasing/rebuilding on a patch I just committed, which uses this MLTAL again. Hopefully not too bad to propagate.
================
Comment at: clang/lib/AST/ASTContext.cpp:3045
+static bool
+getCanonicalTemplateArguments(const ASTContext &C,
+ ArrayRef<TemplateArgument> OrigArgs,
----------------
typically we name these sorts of things based on their return type, so this would be more `hasCanonicalTemplateArguments`. BUT the modifying nature of it makes this an awkward function all the same.
I think I'd suggest putting both of the 'output's here on the same side of the function, either both a return, or both an out-param, so the name isn't ambiguous/confusing like this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134604/new/
https://reviews.llvm.org/D134604
More information about the cfe-commits
mailing list