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

[Manuel Klimek]

On Mon, Oct 15, 2012 at 8:29 PM, Richard Trieu <rtrieu at google.com> wrote:

> Phillip Craig was working on a patch for decl dumping and I was waiting
> for it to go in so he wouldn't have to deal with the new color and
> indenting code I added.


Cool, makes sense. Waiting impatiently for Phillip's patch then :)

Thanks,
/Manuel


>
>
> On Mon, Oct 15, 2012 at 3:02 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> Hi Richard,
>>
>> has there been any progress on this?
>>
>> Cheers,
>> /Manuel
>>
>>
>> On Fri, Aug 31, 2012 at 12:06 AM, Richard Trieu <rtrieu at google.com>wrote:
>>
>>> 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
>>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121015/5649643c/attachment.html>