[Lldb-commits] [PATCH] D134604: [clang] Implement sugared substitution changes to infrastructure

[Erich Keane via Phabricator via lldb-commits]

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