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

[Philip Craig]

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.

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