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