<div dir="ltr">On Wed, Jan 23, 2013 at 10:02 PM, Philip Craig <span dir="ltr"><<a href="mailto:philipjcraig@gmail.com" target="_blank">philipjcraig@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On 23 January 2013 23:12, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>

> On Wed, Jan 23, 2013 at 12:04 PM, Philip Craig <<a href="mailto:philipjcraig@gmail.com">philipjcraig@gmail.com</a>><br>
> wrote:<br>
>><br>
>><br>
>>   Does this still need updating for the comment dumping? (and the color<br>
>> patch too)<br>
>><br>
>>   I don't like how fiddly the lastChild() handling is, but I can't think<br>
>> of any alternative besides buffering the output, and I'm not sure doing that<br>
>> would be worth the effort.<br>
>><br>
>>   Could you include more context in future patches please.<br>
>><br>
>>   Manuel, can Phabricator be changed to display a separator when there are<br>
>> missing lines due to limited context?<br>
><br>
><br>
> Can you point me at where it doesn't? Also, in general, consider uploading<br>
> patches with a higher context (see <a href="http://llvm.org/docs/Phabricator.html" target="_blank">http://llvm.org/docs/Phabricator.html</a>).<br>
<br>
</div>For example, between lines 75 and 79 in this patch.<br></blockquote><div><br></div><div style>I filed: <a href="https://secure.phabricator.com/T2407">https://secure.phabricator.com/T2407</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

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