[cfe-commits] [Patch] Visually update -ast-dump

[Richard Trieu]

On Thu, Aug 30, 2012 at 1:06 PM, Alexander Kornienko <alexfh at google.com>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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120830/c6defe22/attachment.html>