Essentially, I tried to do several reviews, and I found it very hard to separate the diff from the comments at a glance. This is *particularly* problematic when you start getting unchanged context in the snippet of code. I know the email you responded to had every line prefixed with a "+", but imagine comments on unchanged code asking "why didn't you update this?" There is nothing to really differentiate between the code and the comment at that point.<div>
<br></div><div>The other factor in my thinking is this: I'd rather err slightly on the side of a bit much ascii art delineating things. If we assume that we fail to make everyone happy, having a bit of ascii-art fluff in the way of someone who isn't interested isn't likely to confuse them or seriously degrade the ease of reading the comment mail. On the other hand, someone who struggles to see the difference between the patch and the comments may be really confused and waste quite a bit of time trying to orient themselves without some lines to help.</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 21, 2012 at 5:00 AM, Sean Silva <span dir="ltr"><<a href="mailto:silvas@purdue.edu" target="_blank">silvas@purdue.edu</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ah, ok, I didn't see the thread where chandlerc asked for it.<br>
<br>
chandlerc, could you briefly reiterate why you asked for it?<br>
<span class="HOEnZb"><font color="#888888"><br>
--Sean Silva<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Mon, Aug 20, 2012 at 11:46 PM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> +chandlerc<br>
><br>
> fight!<br>
><br>
> (you're basically telling me to take out exactly what Chandler requested :)<br>
><br>
> Perhaps we should also take this onto cfe-dev?<br>
><br>
> On Mon, Aug 20, 2012 at 8:11 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br>
>> Phabricator feedback:<br>
>><br>
>>> ================<br>
>>> Comment at: loop-convert/LoopActions.cpp:836<br>
>>> @@ +835,3 @@<br>
>>> +<br>
>>> +  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());<br>
>>> +  // We require that a single array/container be indexed into by LoopVar.<br>
>>> ----------------<br>
>><br>
>> This seems pretty heavily marked-up. I think we could do without the<br>
>> `====`, `--------`, and `@@...@@` to make it a bit more<br>
>> "human-readable". Also, I think an extra newline between the diff<br>
>> excerpt and the comment would aid in readability as well.<br>
>><br>
>> --Sean Silva<br>
>><br>
>> On Mon, Aug 20, 2012 at 11:05 PM, Sean Silva<br>
>> <<a href="mailto:reviews@llvm-reviews.chandlerc.com">reviews@llvm-reviews.chandlerc.com</a>> wrote:<br>
>>><br>
>>><br>
>>> ================<br>
>>> Comment at: loop-convert/LoopActions.cpp:836<br>
>>> @@ +835,3 @@<br>
>>> +<br>
>>> +  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());<br>
>>> +  // We require that a single array/container be indexed into by LoopVar.<br>
>>> ----------------<br>
>>> You seem to have a lot of these bare calls to `std::min()` to compute the `ConfidenceLevel` all over the place. Could you encapsulate this operation and give it a descriptive name?<br>
>>><br>
>>> ================<br>
>>> Comment at: loop-convert/LoopMatchers.cpp:17<br>
>>> @@ +16,3 @@<br>
>>> +<br>
>>> +// FIXME: How best to document complicated matcher expressions? They're fairly<br>
>>> +// self-documenting...but there may be some unintuitive parts.<br>
>>> ----------------<br>
>>> I think that most of these "matcher factory" functions would benefit from a documentation comment containing:<br>
>>><br>
>>> * A list of bound variable names (or whatever you call the things captured by `id()` and `.bind()`)<br>
>>> * A chunk of example code with the part that would be matched called out in some way (maybe surround it with `@@@`, `|||` or some other marker which will clearly stand out)<br>
>>><br>
>>> Also, for these in particular, I would like to see a brief explanation of how they fit into the larger for-loop conversion process.<br>
>>><br>
>>><br>
>>> ================<br>
>>> Comment at: loop-convert/LoopMatchers.cpp:21<br>
>>> @@ +20,3 @@<br>
>>> +StatementMatcher makeArrayLoopMatcher() {<br>
>>> +  static StatementMatcher LHSMatcher =<br>
>>> +  expression(ignoringImpCasts(declarationReference(to(<br>
>>> ----------------<br>
>>> Why are these static? (are they generating static constructors... ew...).<br>
>>><br>
>>> ================<br>
>>> Comment at: test/loop-convert/Inputs/structures.h:5<br>
>>> @@ +4,3 @@<br>
>>> +extern "C" {<br>
>>> +extern int printf(const char *restrict, ...);<br>
>>> +}<br>
>>> ----------------<br>
>>> I don't think it's legal to use `restrict` in C++, even in `extern "C"`.<br>
>>><br>
>>><br>
>>> <a href="http://llvm-reviews.chandlerc.com/D19" target="_blank">http://llvm-reviews.chandlerc.com/D19</a><br>
</div></div></blockquote></div><br></div>