Hi Richard,<div><br></div><div>has there been any progress on this?</div><div><br></div><div>Cheers,</div><div>/Manuel</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 31, 2012 at 12:06 AM, 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 class="gmail_quote"><div class="im">On Thu, Aug 30, 2012 at 1:06 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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/" target="_blank">http://llvm-reviews.chandlerc.com/</a></div>
</blockquote></div><div>I haven't been paying attention to the review options for this mailing list. I'll try sending future versions of this patch there.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div> ;):</div></blockquote><div> <a href="http://xkcd.com/541/" target="_blank">http://xkcd.com/541/</a></div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div><div><font face="courier new, monospace">===================================================================</font></div><div><font face="courier new, monospace">--- lib/AST/StmtDumper.cpp<span style="white-space:pre-wrap"> </span>(revision 162862)</font></div>
<div><font face="courier new, monospace">+++ lib/AST/StmtDumper.cpp<span style="white-space:pre-wrap"> </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></blockquote>
</div><div>It came when I copied the colors from TextDiagnostic.cpp. I don't think they are necessary in either spot.</div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote></div><div>Yes. </div><div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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></div></blockquote></div><div>Either way works. </div>
<div class="im"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><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></blockquote></div><div>I did notice the FIXME's in DumpDeclarator(). I'm aware of it, but haven't started any work on it. I'll ping you if I start working in that direction. Are you considering starting work on it soon?</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div></div><div><span><font color="#888888"><br clear="all"><span class="HOEnZb"><font color="#888888"><div><br></div>
-- <br>
</font></span></font></span></div></div><span class="HOEnZb"><font color="#888888"><span><font color="#888888"><div>Regards,</div><div>Alex</div>
</font></span></font></span></blockquote></div><br>
<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></div>