[PATCH] D55491: Implement TemplateArgument dumping in terms of Visitor

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/TemplateArgumentVisitor.h:23
+
+template <typename T> struct make_ref { using type = T &; };
+template <typename T> struct make_const_ref { using type = const T &; };
----------------
`std::add_lvalue_reference<>` already does this, so do we really need this?


================
Comment at: include/clang/AST/TemplateArgumentVisitor.h:24
+template <typename T> struct make_ref { using type = T &; };
+template <typename T> struct make_const_ref { using type = const T &; };
+
----------------
I'm guessing we can't use `std::add_lvalue_reference<std::add_const<>>` for the same reasons we couldn't use it for pointers, but then `make_const_ref` should live in STLExtras.h and not here (it's generally useful).


================
Comment at: include/clang/AST/TemplateArgumentVisitor.h:27-28
+/// A simple visitor class that helps create template argument visitors.
+template <template <typename> class Ref, typename ImplClass,
+          typename RetTy = void, class... ParamTys>
+class Base {
----------------
s/class/typename to be self-consistent


================
Comment at: include/clang/AST/TemplateArgumentVisitor.h:55
+
+  RetTy VisitNullTemplateArgument(REF(TemplateArgument) TA, ParamTys... P) {
+    return VisitTemplateArgument(TA, std::forward<ParamTys>(P)...);
----------------
Might be worth it to replace these with a macro, sort of like how `DISPATCH` is used above.


================
Comment at: include/clang/AST/TextNodeDumper.h:156
+  void Visit(const TemplateArgument &TA, SourceRange R,
+             const Decl *From = nullptr, const char *label = nullptr);
+
----------------
label -> Label and I think we should use a `StringRef` for the type.


================
Comment at: lib/AST/ASTDumper.cpp:449-450
+    void VisitPackTemplateArgument(const TemplateArgument &TA) {
+      for (TemplateArgument::pack_iterator I = TA.pack_begin(),
+                                           E = TA.pack_end();
+           I != E; ++I)
----------------
`llvm::for_each(TA.pack_elements(), ...);` (or a range-based for loop)


================
Comment at: lib/AST/TextNodeDumper.cpp:68
+void TextNodeDumper::Visit(const TemplateArgument &TA, SourceRange R,
+                           const Decl *From, const char *label) {
+  OS << "TemplateArgument";
----------------
label -> Label


================
Comment at: lib/AST/TextNodeDumper.cpp:73
+
+  if (From) {
+    dumpDeclRef(From, label);
----------------
Elide braces


Repository:
  rC Clang

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

https://reviews.llvm.org/D55491