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?


================
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

Reply via email to