[PATCH] D56642: NFC: Move dump of type nodes to NodeDumper

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Mostly looks good (a few minor nits), but I did have a design question about whether we should prefer calling a `Visit` method for type hierarchies instead of reimplementing the functionality.



================
Comment at: lib/AST/ASTDumper.cpp:145
     void VisitVariableArrayType(const VariableArrayType *T) {
-      OS << " ";
-      NodeDumper.dumpSourceRange(T->getBracketsRange());
-      VisitArrayType(T);
+      dumpTypeAsChild(T->getElementType());
       dumpStmt(T->getSizeExpr());
----------------
Why this approach instead of deferring to `VisitArrayType()` as before? I prefer calling the Visit function rather than reimplementing the functionality because we may decide to later improve the base class printing and expect subclasses to automatically pick that up. WDYT?


================
Comment at: lib/AST/ASTDumper.cpp:164
     void VisitFunctionProtoType(const FunctionProtoType *T) {
-      auto EPI = T->getExtProtoInfo();
-      if (EPI.HasTrailingReturn) OS << " trailing_return";
-
-      if (!T->getTypeQuals().empty())
-        OS << " " << T->getTypeQuals().getAsString();
-
-      switch (EPI.RefQualifier) {
-        case RQ_None: break;
-        case RQ_LValue: OS << " &"; break;
-        case RQ_RValue: OS << " &&"; break;
-      }
-      // FIXME: Exception specification.
-      // FIXME: Consumed parameters.
-      VisitFunctionType(T);
+      dumpTypeAsChild(T->getReturnType());
       for (QualType PT : T->getParamTypes())
----------------
Why this approach as opposed to deferring to `VisitFunctionType()` as before?


================
Comment at: lib/AST/TextNodeDumper.cpp:15
 #include "clang/AST/TextNodeDumper.h"
 
+#include "clang/AST/DeclTemplate.h"
----------------
Remove newline


================
Comment at: lib/AST/TextNodeDumper.cpp:960
+void TextNodeDumper::VisitFunctionType(const FunctionType *T) {
+  auto EI = T->getExtInfo();
+  if (EI.getNoReturn())
----------------
It'd be nice to remove this use of `auto`.


================
Comment at: lib/AST/TextNodeDumper.cpp:971
+void TextNodeDumper::VisitFunctionProtoType(const FunctionProtoType *T) {
+  auto EPI = T->getExtProtoInfo();
+  if (EPI.HasTrailingReturn)
----------------
It'd be nice to remove this use of `auto`.


================
Comment at: lib/AST/TextNodeDumper.cpp:1047
+void TextNodeDumper::VisitPackExpansionType(const PackExpansionType *T) {
+  if (auto N = T->getNumExpansions())
+    OS << " expansions " << *N;
----------------
It'd be nice to remove this use of `auto`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56642