On Wed, Jan 23, 2013 at 10:02 PM, Philip Craig <[email protected]>wrote:
> On 23 January 2013 23:12, Manuel Klimek <[email protected]> wrote: > > On Wed, Jan 23, 2013 at 12:04 PM, Philip Craig <[email protected]> > > 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 > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
