On Thu, Aug 30, 2012 at 1:06 PM, Alexander Kornienko <[email protected]>wrote:
> Looks nice! > > 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 > http://llvm-reviews.chandlerc.com/ > I haven't been paying attention to the review options for this mailing list. I'll try sending future versions of this patch there. > ;): > http://xkcd.com/541/ > > =================================================================== > --- lib/AST/StmtDumper.cpp (revision 162862) > +++ lib/AST/StmtDumper.cpp (working copy) > ... > + static const enum raw_ostream::Colors lineColor = > Are there reasons not to omit "enum" in "enum raw_ostream::Colors"? > It came when I copied the colors from TextDiagnostic.cpp. I don't think they are necessary in either spot. > > ... > + // Indicates whether more child are expected at the current tree depth > You mean "more *children*"? > Yes. > > ... > @@ -62,28 +105,74 @@ > // Print out children. > Stmt::child_range CI = S->children(); > if (CI) { > + Indents.push_back(IT_Child); > + Stmt *TempStmt; > Do you need to declare TempStmt here, and not where it's assigned? > Either way works. > > while (CI) { > OS << '\n'; > - DumpSubTree(*CI++); > + TempStmt = *CI++; > I mean here ^ > + if (!CI) > + Indents.back() = IT_LastChild; > + DumpSubTree(TempStmt); > } > + Indents.pop_back(); > > 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 ;) > > 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? > > > -- > Regards, > Alex >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
