[cfe-dev] Phabricator review mails

[Chandler Carruth]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120821/ab8c0b16/attachment.html>