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). 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
