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