[cfe-dev] Phabricator review mails

[Sean Silva]

All good points. I didn't consider the case of commenting on code
without the +/-, as that does really make it ambiguous. With that in
perspective, I agree with you.

--Sean Silva

On Tue, Aug 21, 2012 at 8:18 AM, Chandler Carruth <chandlerc at google.com> wrote:
> 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.
>
> 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.
>
>
> On Tue, Aug 21, 2012 at 5:00 AM, Sean Silva <silvas at purdue.edu> wrote:
>>
>> 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
>
>