[cfe-dev] Phabricator review mails

[Sean Silva]

Ah, ok, I didn't see the thread where chandlerc asked for it.

chandlerc, could you briefly reiterate why you asked for it?

--Sean Silva

On Mon, Aug 20, 2012 at 11:46 PM, Manuel Klimek <klimek at google.com> wrote:
> +chandlerc
>
> fight!
>
> (you're basically telling me to take out exactly what Chandler requested :)
>
> Perhaps we should also take this onto cfe-dev?
>
> On Mon, Aug 20, 2012 at 8:11 PM, Sean Silva <silvas at purdue.edu> wrote:
>> Phabricator feedback:
>>
>>> ================
>>> Comment at: loop-convert/LoopActions.cpp:836
>>> @@ +835,3 @@
>>> +
>>> +  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());
>>> +  // We require that a single array/container be indexed into by LoopVar.
>>> ----------------
>>
>> This seems pretty heavily marked-up. I think we could do without the
>> `====`, `--------`, and `@@...@@` to make it a bit more
>> "human-readable". Also, I think an extra newline between the diff
>> excerpt and the comment would aid in readability as well.
>>
>> --Sean Silva
>>
>> On Mon, Aug 20, 2012 at 11:05 PM, Sean Silva
>> <reviews at llvm-reviews.chandlerc.com> wrote:
>>>
>>>
>>> ================
>>> Comment at: loop-convert/LoopActions.cpp:836
>>> @@ +835,3 @@
>>> +
>>> +  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());
>>> +  // We require that a single array/container be indexed into by LoopVar.
>>> ----------------
>>> 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?
>>>
>>> ================
>>> Comment at: loop-convert/LoopMatchers.cpp:17
>>> @@ +16,3 @@
>>> +
>>> +// FIXME: How best to document complicated matcher expressions? They're fairly
>>> +// self-documenting...but there may be some unintuitive parts.
>>> ----------------
>>> I think that most of these "matcher factory" functions would benefit from a documentation comment containing:
>>>
>>> * A list of bound variable names (or whatever you call the things captured by `id()` and `.bind()`)
>>> * 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)
>>>
>>> Also, for these in particular, I would like to see a brief explanation of how they fit into the larger for-loop conversion process.
>>>
>>>
>>> ================
>>> Comment at: loop-convert/LoopMatchers.cpp:21
>>> @@ +20,3 @@
>>> +StatementMatcher makeArrayLoopMatcher() {
>>> +  static StatementMatcher LHSMatcher =
>>> +  expression(ignoringImpCasts(declarationReference(to(
>>> ----------------
>>> Why are these static? (are they generating static constructors... ew...).
>>>
>>> ================
>>> Comment at: test/loop-convert/Inputs/structures.h:5
>>> @@ +4,3 @@
>>> +extern "C" {
>>> +extern int printf(const char *restrict, ...);
>>> +}
>>> ----------------
>>> I don't think it's legal to use `restrict` in C++, even in `extern "C"`.
>>>
>>>
>>> http://llvm-reviews.chandlerc.com/D19