[PATCH] D55188: Extract TextChildDumper class

[Stephen Kelly via Phabricator via cfe-commits]

steveire marked 2 inline comments as done.
steveire added inline comments.


================
Comment at: include/clang/AST/ASTDumperUtils.h:22
+
+namespace ast_dumper {
+// Colors used for various parts of the AST dump
----------------
aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > I'm not certain this namespace is useful, especially when it gets imported at TU scope in ASTDumper.cpp.
> > > it gets imported at TU scope in ASTDumper.cpp
> > 
> > Today that's the only place it is used. In the future that won't be true.
> Then we can add the namespace in the future when we find a situation where we're worried about collisions? As it stands, this only provides the illusion of safety. If you really want to keep it, please pull the global using declaration.
There is no need for such churn as adding the namespace later.

The `using` I added is beside two other `using`s in a cpp file. There is nothing 'global' about it.


================
Comment at: include/clang/AST/ASTDumperUtils.h:96
+
+struct TextChildDumper {
+  raw_ostream &OS;
----------------
aaron.ballman wrote:
> I'm not sold on the name for this class. It's a bit too generic to understand what it does. How about `ASTDumpLayoutFormatter` (and `ASTDumpNodeFormatter` for the node dumper)?
I'll rename the class to `ASTTextTreeStructure`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55188