Looks nice!<div><br>I'm not the one to approve it, but here are few minor comments regarding the patch (btw, it would be much more convenient with <a href="http://llvm-reviews.chandlerc.com/">http://llvm-reviews.chandlerc.com/</a> ;):</div>
<div><br></div><div><div><font face="courier new, monospace">===================================================================</font></div><div><font face="courier new, monospace">--- lib/AST/StmtDumper.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 162862)</font></div>
<div><font face="courier new, monospace">+++ lib/AST/StmtDumper.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</font></div></div><div><font face="courier new, monospace">...</font></div><div>
<div><font face="courier new, monospace">+ static const enum raw_ostream::Colors lineColor =</font></div></div><div>Are there reasons not to omit "enum" in "enum raw_ostream::Colors"?</div><div><br>
</div><div><font face="courier new, monospace">...</font></div><div><div><font face="courier new, monospace">+ // Indicates whether more child are expected at the current tree depth</font></div><div>You mean "more <i>children</i>"?</div>
<div><br></div><div><font face="courier new, monospace">...</font></div><div><div><font face="courier new, monospace">@@ -62,28 +105,74 @@</font></div><div><font face="courier new, monospace"> // Print out children.</font></div>
<div><font face="courier new, monospace"> Stmt::child_range CI = S->children();</font></div><div><font face="courier new, monospace"> if (CI) {</font></div><div><font face="courier new, monospace">+ Indents.push_back(IT_Child);</font></div>
<div><font face="courier new, monospace">+ Stmt *TempStmt;</font></div><div>Do you need to declare TempStmt here, and not where it's assigned?</div><div><br></div><div><font face="courier new, monospace"> while (CI) {</font></div>
<div><font face="courier new, monospace"> OS << '\n';</font></div><div><font face="courier new, monospace">- DumpSubTree(*CI++);</font></div><div><font face="courier new, monospace">+ TempStmt = *CI++;</font></div>
<div>I mean here ^</div><div><font face="courier new, monospace">+ if (!CI)</font></div><div><font face="courier new, monospace">+ Indents.back() = IT_LastChild;</font></div><div><font face="courier new, monospace">+ DumpSubTree(TempStmt);</font></div>
<div><font face="courier new, monospace"> }</font></div><div><font face="courier new, monospace">+ Indents.pop_back();</font></div></div><div><br></div><div>Besides that, are you going to deal with AST dumping further? I was planning to start replacing current Decl dumping with something more informative (based on current -ast-dump-xml implementation, probably) at some point. It would be nice to try to avoid conflicts ;)</div>
<div><br></div><div><br><div class="gmail_quote">On Wed, Aug 29, 2012 at 7:11 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>For too long, -ast-dump would produce drab, monochromatic, textual AST's. Large walls of text to slough through to find the interesting bits. Worse, if a statement filled more than one line, the structural information of the tree would be easily lost amid the type names, source locations, pointer address, and statement names. Fear not, a solution presents itself!</div>
<div><br></div><div>1) The addition of ASCII characters into the unused indents at the beginning of lines. Pipes "|", dashes "-", and backslashes "\" quickly show relationships between the different statements. Find siblings, parents, and children fast and easy, never lose your place again.</div>
<div><br></div><div>2) For each statement, different parts are highlighted with different colors. This visually separates the components and allows faster differentiating. Looking for a specific type? All types are the same color. Same for the statement names. And source locations.</div>
<div><br></div><div>3) But wait, there's more! Since -ast-dump and dump() use the same ASTVisitor, you can also get colors in your debugger! Just use dumpColor() instead of dump() and get colors while you debug.</div>
<div><br></div><div>To use, either use "clang -Xclang -ast-dump" or "clang -cc1 -ast-dump -fcolor-diagnostics" from a color-enabled terminal.</div><div><br></div><div>Implementation detail:</div><div>
Color changes were handled by changeColor() and resetColor() from the raw_ostream. The different colors are stored in constants at the top of the class before being used in the relevant visit method.</div>
<div><br></div><div>Tree structure printing was handled by a vector of statuses. The status indicates if there's future children at the level of the index. This is enough information to generate the tree structure. This replaces the integer indent counter.</div>
<div><br></div><div>To determine if color printing is required, the dumper queries the DiagnosticEngine in the SourceManager. This is enough for -ast-dump. However, when debugging and call Stmt.dump(), the Stmt doesn't have a SourceManager. It is possible to call dump() with a SourceManager, but to simplify things, a dumpColor() was created to force color printing.</div>
<div><br></div><div>I've attached a patch and an image showing the difference between color and no color. Work in progress, comments welcome.</div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>
</div></div><div>Regards,</div><div>Alex</div>