[cfe-commits] [PATCH] Utilize the indents in -ast-dump to draw structural information

[Manuel Klimek]

On Wed, Jan 23, 2013 at 10:02 PM, Philip Craig <philipjcraig at gmail.com>wrote:

> On 23 January 2013 23:12, Manuel Klimek <klimek at google.com> wrote:
> > On Wed, Jan 23, 2013 at 12:04 PM, Philip Craig <philipjcraig at gmail.com>
> > wrote:
> >>
> >>
> >>   Does this still need updating for the comment dumping? (and the color
> >> patch too)
> >>
> >>   I don't like how fiddly the lastChild() handling is, but I can't think
> >> of any alternative besides buffering the output, and I'm not sure doing
> that
> >> would be worth the effort.
> >>
> >>   Could you include more context in future patches please.
> >>
> >>   Manuel, can Phabricator be changed to display a separator when there
> are
> >> missing lines due to limited context?
> >
> >
> > Can you point me at where it doesn't? Also, in general, consider
> uploading
> > patches with a higher context (see http://llvm.org/docs/Phabricator.html
> ).
>
> For example, between lines 75 and 79 in this patch.
>

I filed: https://secure.phabricator.com/T2407


>
> >
> > Cheers,
> > /Manuel
> >
> >>
> >>
> >>
> >> ================
> >> Comment at: lib/AST/ASTDumper.cpp:40
> >> @@ +39,3 @@
> >> +
> >> +    /// Indents - Indents[i] indicates if another child exists at level
> >> i.
> >> +    /// Used by Indent() to print the tree structure.
> >> ----------------
> >> Don't put the name at the beginning of the comment.
> >>
> >> ================
> >> Comment at: lib/AST/ASTDumper.cpp:541
> >> @@ -445,1 +540,3 @@
> >> +  setMoreChildren(HasDeclContext);
> >> +  if (HasAttrs) {
> >>      for (AttrVec::const_iterator I = D->getAttrs().begin(),
> >> ----------------
> >> I think this test is some code I forgot to delete. The iterator check in
> >> the for loop is enough, so no need for this or the hasAttrs() call
> above.
> >>
> >> ================
> >> Comment at: lib/AST/ASTDumper.cpp:549
> >> @@ -448,2 +548,3 @@
> >>    }
> >>    // Decls within functions are visited by the body
> >> +  setMoreChildren(false);
> >> ----------------
> >> This comment might make more sense if it is moved up to HasDeclContext?
> >>
> >> ================
> >> Comment at: lib/AST/ASTDumper.cpp:626
> >> @@ -519,3 +625,3 @@
> >>
> >> -  if (const FunctionTemplateSpecializationInfo *FTSI =
> >> -      D->getTemplateSpecializationInfo())
> >> +  bool oldMoreChildren = hasMoreChildren();
> >> +  const FunctionTemplateSpecializationInfo *FTSI =
> >> ----------------
> >> Use upper case for variables. Elsewhere too.
> >>
> >>
> >> http://llvm-reviews.chandlerc.com/D281
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130125/6f254dd5/attachment.html>