llvm dev - Nov 2019 - [RFC] High-Level Code-Review Documentation Update

On Mon, Nov 18, 2019 at 4:53 PM Finkel, Hal J. via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> On 11/18/19 4:29 AM, James Henderson wrote:
>>
>> Only a single LGTM is required.  Reviewers are expected to only LGTM
>> patches they're confident in their knowledge of.  Reviewers may
review
>> and provide suggestions, but explicitly defer LGTM to someone else.
>> This is encouraged and a good way for new contributors to learn the
code.
>
> Whilst I get what you're trying to say, I'm not particularly
comfortable with this particular suggestion as it stands: I regularly am one of
two or three active reviewers on something, often spread out over different
timezones, and I might be happy with it, in which case I'd signal that with
an LGTM, but others might not be ready for it to be committed. Sometimes
I'll ask for the author to wait to commit until one or more of the other
reviewers are happy too, but other times I forget to explicitly say this.
Perhaps a couple of sentences could be added to the end of this paragraph to
capture this approach:
>
> "If multiple reviewers have been actively reviewing the patch, it is
generally courteous to allow all of them a chance to give their LGTM before
committing, after one LGTM has been received. The reviewer who gives the
original LGTM may suggest an appropriate amount of time to wait before
committing in this case."
>
> What I want to avoid is me (UK timezone) making some suggestions on a patch
proposed by someone e.g. in the US, then a reviewer from the US getting into an
active discussion, proposing a counter-suggestion, which gets adopted and LGTMed
by that reviewer, resulting in a commit before I've had a chance to follow
up on my comments etc. Obviously I can make post-commit requests, but sometimes
it feels like the bar for suggestions post-commit is higher, and therefore my
comments might not reach that level etc.
>
>
> I agree with this. I was planning on proposing wording along the lines of
the following, adding to the original suggestion:
>
> When providing an unqualified LGTM (approval to commit), it is the
responsibility of the reviewer to have reviewed all of the discussion and
feedback from all reviewers ensuring that all feedback has been addressed and
that all other reviewers will almost surely be satisfied with the patch being
approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
"LGTM, but please wait for @someone, @someone_else"). You may also do
this if you are fairly certain that a particular community member will wish to
review, even if that person hasn't done so yet (although don't wait for
more than one week if that person has not responded; if you think something is
"must see" by a wider audience, it should have an RFC). If it is
likely that others will want to review a recently-posted patch, especially if
there might be objections, but no one else has done so yet, it is also polite to
provide a qualified approval (e.g., "LGTM, but please wait for a couple
days in case others wish to review").
On a somewhat related note: Due to timezones, I think good etiquette
is to wait a reasonable time before committing a patch if you receive
a LGTM very quickly. I think non-trivial patches should generally be
up for at least 24 hours -- maybe even extend that to 48 hours --
before committing.

Of course, this is usually not an issue in LLVM today because we're so
bad at reviewing code in general :) If you've waited on a review for a
month and pinged people several times, then by all means feel free to
commit immediately after getting a LGTM.

Cheers,
Nicolai


>
> Thoughts?
>
>  -Hal
>
>
>
> James
>
> On Sat, 16 Nov 2019 at 16:37, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>>
>> + 1 in general, a couple of suggestions
>>
>> On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
>> > Hi, everyone,
>> >
>> > I've been fielding an increasing number of questions about how
our
>> > code-review process in LLVM works from people who are new to our
>> > community, and it's been pointed out to me that our
documentation on
>> > code reviews is both out of date and not as helpful as it could be
to
>> > new developers.
>> >
>> >    http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>> >
>> > I would like to compose a patch to update this, but before I do
that, I
>> > want to highlight some of my thoughts to get feedback. My intent
is to
>> > capture our community best practices in writing so that people new
to
>> > our community understand our processes and expectations. Here are
some
>> > things that I would like to capture:
>> >
>> >   1. You do not need to be an expert in some area of the compiler
to
>> > review patches; it's fine to ask questions about what some
piece of code
>> > is doing. If it's not clear to you what is going on,
you're unlikely to
>> > be the only one. Extra comments and/or test cases can often help
(and
>> > asking for comments in the test cases is fine as well).
>> Authors are encouraged to interpret questions as reasons to reexamine
>> the readability of the code in question.  Structural changes, or
further
>> comments may be appropriate.
>> >
>> >   2. If you review a patch, but don't intend for the review
process to
>> > block on your approval, please state that explicitly. Out of
courtesy,
>> > we generally wait on committing a patch until all reviewers are
>> > satisfied, and if you don't intend to look at the patch again
in a
>> > timely fashion, please communicate that fact in the review.
>> >
>> >   3. All comments by reviewers should be addressed by the patch
author.
>> > It is generally expected that suggested changes will be
incorporated
>> > into the next revision of the patch unless the author and/or other
>> > reviewers can articulate a good reason to do otherwise (and then
the
>> > reviewers must agree). If you suggest changes in a code review,
but
>> > don't wish the suggestion to be interpreted this strongly,
please state
>> > so explicitly.
>> >
>> >   4. Reviewers may request certain aspects of a patch to be broken
out
>> > into separate patches for independent review, and also, reviewers
may
>> > accept a patch conditioned on the author providing a follow-up
patch
>> > addressing some particular issue or concern (although no committed
patch
>> > should leave the project in a broken state). Reviewers can also
accept a
>> > patch conditioned on the author applying some set of minor updates
prior
>> > to committing, and when applicable, it is polite for reviewers to
do so.
>> >
>> >   5. Aim to limit the number of iterations in the review process.
For
>> > example, when suggesting a change, if you want the author to make
a
>> > similar set of changes at other places in the code, please explain
the
>> > requested set of changes so that the author can make all of the
changes
>> > at once. If a patch will require multiple steps prior to approval
(e.g.,
>> > splitting, refactoring, posting data from specific performance
tests),
>> > please explain as many of these up front as possible. This allows
the
>> > patch author to make the most-efficient use of his or her time.
>> If the path forward is not clear - because the patch is too large to
>> meaningful review, or direction needs to be settled - it is fine to
>> suggest a clear next step (e.g. landing a refactoring) followed by a
>> re-review.  Please state explicitly if the path forward is unclear to
>> prevent confusions on the part of the author.
>> >
>> >   6. Some changes are too large for just a code review. Changes
that
>> > should change the Language Reference (e.g., adding new
>> > target-independent intrinsics), adding language extensions in
Clang, and
>> > so on, require an RFC on *-dev first. For changes that promise
>> > significant impact on users and/or downstream code bases,
reviewers can
>> > request an RFC (Request for Comment) achieving consensus before
>> > proceeding with code review. That having been said, posting
initial
>> > patches can help with discussions on an RFC.
>> >
>> > Lastly, the current text reads, "Code reviews are conducted
by email on
>> > the relevant project’s commit mailing list, or alternatively on
the
>> > project’s development list or bug tracker.", and then only
later
>> > mentions Phabricator. I'd like to move Phabricator to be
mentioned on
>> > this line before the other methods.
>> >
>> > Please let me know what you think.
>> >
>> > Thanks again,
>> >
>> > Hal
>>
>> A couple of additional things:
>>
>> Only a single LGTM is required.  Reviewers are expected to only LGTM
>> patches they're confident in their knowledge of.  Reviewers may
review
>> and provide suggestions, but explicitly defer LGTM to someone else.
>> This is encouraged and a good way for new contributors to learn the
code.
>>
>> There is a cultural expectation that at least one reviewer is from a
>> different organization than the author of the patch.  If that's not
>> possible, care should be taken to ensure overall direction has been
>> widely accepted.
>>
>> Post commit review is encouraged via either phabricator or email. 
There
>> is a strong expectation that authors respond promptly to post commit
>> feedback and address it.  Failure to do so is cause for the patch to be
>> reverted.  If substantial problems are identified, it is expected that
>> the patch is reverted, fixed offline, and then recommitted (possibly
>> after further review.)
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

On Wed, Nov 20, 2019 at 2:28 AM Nicolai Hähnle via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Mon, Nov 18, 2019 at 4:53 PM Finkel, Hal J. via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > On 11/18/19 4:29 AM, James Henderson wrote:
> >>
> >> Only a single LGTM is required.  Reviewers are expected to only
LGTM
> >> patches they're confident in their knowledge of.  Reviewers
may review
> >> and provide suggestions, but explicitly defer LGTM to someone
else.
> >> This is encouraged and a good way for new contributors to learn
the
> code.
> >
> > Whilst I get what you're trying to say, I'm not particularly
comfortable
> with this particular suggestion as it stands: I regularly am one of two or
> three active reviewers on something, often spread out over different
> timezones, and I might be happy with it, in which case I'd signal that
with
> an LGTM, but others might not be ready for it to be committed. Sometimes
> I'll ask for the author to wait to commit until one or more of the
other
> reviewers are happy too, but other times I forget to explicitly say this.
> Perhaps a couple of sentences could be added to the end of this paragraph
> to capture this approach:
> >
> > "If multiple reviewers have been actively reviewing the patch, it
is
> generally courteous to allow all of them a chance to give their LGTM before
> committing, after one LGTM has been received. The reviewer who gives the
> original LGTM may suggest an appropriate amount of time to wait before
> committing in this case."
> >
> > What I want to avoid is me (UK timezone) making some suggestions on a
> patch proposed by someone e.g. in the US, then a reviewer from the US
> getting into an active discussion, proposing a counter-suggestion, which
> gets adopted and LGTMed by that reviewer, resulting in a commit before
I've
> had a chance to follow up on my comments etc. Obviously I can make
> post-commit requests, but sometimes it feels like the bar for suggestions
> post-commit is higher, and therefore my comments might not reach that level
> etc.
> >
> >
> > I agree with this. I was planning on proposing wording along the lines
> of the following, adding to the original suggestion:
> >
> > When providing an unqualified LGTM (approval to commit), it is the
> responsibility of the reviewer to have reviewed all of the discussion and
> feedback from all reviewers ensuring that all feedback has been addressed
> and that all other reviewers will almost surely be satisfied with the patch
> being approved. If unsure, the reviewer should provide a qualified
> approval, (e.g., "LGTM, but please wait for @someone,
@someone_else"). You
> may also do this if you are fairly certain that a particular community
> member will wish to review, even if that person hasn't done so yet
> (although don't wait for more than one week if that person has not
> responded; if you think something is "must see" by a wider
audience, it
> should have an RFC). If it is likely that others will want to review a
> recently-posted patch, especially if there might be objections, but no one
> else has done so yet, it is also polite to provide a qualified approval
> (e.g., "LGTM, but please wait for a couple days in case others wish to
> review").
>
> On a somewhat related note: Due to timezones, I think good etiquette
> is to wait a reasonable time before committing a patch if you receive
> a LGTM very quickly. I think non-trivial patches should generally be
> up for at least 24 hours -- maybe even extend that to 48 hours --
> before committing.
>
I'd disagree here - and perhaps my motivation should be enshrined in the
documentation too: I believe a review approval is roughly equivalent to "I
would be comfortable committing this code myself with post-commit review",
so a single approval under those constraints should be able to be committed
immediately & if other reviewers chime in later it can be handled as it
would be with any other post-commit review feedback (which can include
reverting).

If there are "approvals" that aren't from people who have the
authority/experience to commit such a change directly, that's problematic.
(& it does happen, and it is super vague about who has such authority and
who doesn't, etc - but I think for most reviews "if you wrote this
yourself, would you commit it without pre-commit review" should be the bar
for approval (once you get up to more nuanced design discussions, that's
different - and it can sometimes invert "I'd commit this without
review,
but I'm curious what others think/if they have better ideas" (at which
point it's really more up to the submitter to make the judgment call
whether they've got sufficient feedback to help them decide that there
aren't better ideas out there - sometimes that comes quickly in terms of a
single other main developer saying "oh, yeah, that makes total sense - here
are 3 other ideas that come to mind and why they don't seem better than
this" or might be a more ongoing discussion))

> Of course, this is usually not an issue in LLVM today because we're so
> bad at reviewing code in general :) If you've waited on a review for a
> month and pinged people several times, then by all means feel free to
> commit immediately after getting a LGTM.
>
> Cheers,
> Nicolai
>
>
>
> >
> > Thoughts?
> >
> >  -Hal
> >
> >
> >
> > James
> >
> > On Sat, 16 Nov 2019 at 16:37, Philip Reames via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> + 1 in general, a couple of suggestions
> >>
> >> On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
> >> > Hi, everyone,
> >> >
> >> > I've been fielding an increasing number of questions
about how our
> >> > code-review process in LLVM works from people who are new to
our
> >> > community, and it's been pointed out to me that our
documentation on
> >> > code reviews is both out of date and not as helpful as it
could be to
> >> > new developers.
> >> >
> >> >    http://llvm.org/docs/DeveloperPolicy.html#code-reviews
> >> >
> >> > I would like to compose a patch to update this, but before I
do that,
> I
> >> > want to highlight some of my thoughts to get feedback. My
intent is to
> >> > capture our community best practices in writing so that
people new to
> >> > our community understand our processes and expectations. Here
are some
> >> > things that I would like to capture:
> >> >
> >> >   1. You do not need to be an expert in some area of the
compiler to
> >> > review patches; it's fine to ask questions about what
some piece of
> code
> >> > is doing. If it's not clear to you what is going on,
you're unlikely
> to
> >> > be the only one. Extra comments and/or test cases can often
help (and
> >> > asking for comments in the test cases is fine as well).
> >> Authors are encouraged to interpret questions as reasons to
reexamine
> >> the readability of the code in question.  Structural changes, or
further
> >> comments may be appropriate.
> >> >
> >> >   2. If you review a patch, but don't intend for the
review process to
> >> > block on your approval, please state that explicitly. Out of
courtesy,
> >> > we generally wait on committing a patch until all reviewers
are
> >> > satisfied, and if you don't intend to look at the patch
again in a
> >> > timely fashion, please communicate that fact in the review.
> >> >
> >> >   3. All comments by reviewers should be addressed by the
patch
> author.
> >> > It is generally expected that suggested changes will be
incorporated
> >> > into the next revision of the patch unless the author and/or
other
> >> > reviewers can articulate a good reason to do otherwise (and
then the
> >> > reviewers must agree). If you suggest changes in a code
review, but
> >> > don't wish the suggestion to be interpreted this
strongly, please
> state
> >> > so explicitly.
> >> >
> >> >   4. Reviewers may request certain aspects of a patch to be
broken out
> >> > into separate patches for independent review, and also,
reviewers may
> >> > accept a patch conditioned on the author providing a
follow-up patch
> >> > addressing some particular issue or concern (although no
committed
> patch
> >> > should leave the project in a broken state). Reviewers can
also
> accept a
> >> > patch conditioned on the author applying some set of minor
updates
> prior
> >> > to committing, and when applicable, it is polite for
reviewers to do
> so.
> >> >
> >> >   5. Aim to limit the number of iterations in the review
process. For
> >> > example, when suggesting a change, if you want the author to
make a
> >> > similar set of changes at other places in the code, please
explain the
> >> > requested set of changes so that the author can make all of
the
> changes
> >> > at once. If a patch will require multiple steps prior to
approval
> (e.g.,
> >> > splitting, refactoring, posting data from specific
performance tests),
> >> > please explain as many of these up front as possible. This
allows the
> >> > patch author to make the most-efficient use of his or her
time.
> >> If the path forward is not clear - because the patch is too large
to
> >> meaningful review, or direction needs to be settled - it is fine
to
> >> suggest a clear next step (e.g. landing a refactoring) followed by
a
> >> re-review.  Please state explicitly if the path forward is unclear
to
> >> prevent confusions on the part of the author.
> >> >
> >> >   6. Some changes are too large for just a code review.
Changes that
> >> > should change the Language Reference (e.g., adding new
> >> > target-independent intrinsics), adding language extensions in
Clang,
> and
> >> > so on, require an RFC on *-dev first. For changes that
promise
> >> > significant impact on users and/or downstream code bases,
reviewers
> can
> >> > request an RFC (Request for Comment) achieving consensus
before
> >> > proceeding with code review. That having been said, posting
initial
> >> > patches can help with discussions on an RFC.
> >> >
> >> > Lastly, the current text reads, "Code reviews are
conducted by email
> on
> >> > the relevant project’s commit mailing list, or alternatively
on the
> >> > project’s development list or bug tracker.", and then
only later
> >> > mentions Phabricator. I'd like to move Phabricator to be
mentioned on
> >> > this line before the other methods.
> >> >
> >> > Please let me know what you think.
> >> >
> >> > Thanks again,
> >> >
> >> > Hal
> >>
> >> A couple of additional things:
> >>
> >> Only a single LGTM is required.  Reviewers are expected to only
LGTM
> >> patches they're confident in their knowledge of.  Reviewers
may review
> >> and provide suggestions, but explicitly defer LGTM to someone
else.
> >> This is encouraged and a good way for new contributors to learn
the
> code.
> >>
> >> There is a cultural expectation that at least one reviewer is from
a
> >> different organization than the author of the patch.  If
that's not
> >> possible, care should be taken to ensure overall direction has
been
> >> widely accepted.
> >>
> >> Post commit review is encouraged via either phabricator or email. 
There
> >> is a strong expectation that authors respond promptly to post
commit
> >> feedback and address it.  Failure to do so is cause for the patch
to be
> >> reverted.  If substantial problems are identified, it is expected
that
> >> the patch is reverted, fixed offline, and then recommitted
(possibly
> >> after further review.)
> >>
> >>
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> > --
> > Hal Finkel
> > Lead, Compiler Technology and Programming Languages
> > Leadership Computing Facility
> > Argonne National Laboratory
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191120/02611f28/attachment-0001.html>

Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Of course, this is usually not an issue in LLVM today because we're so
> bad at reviewing code in general :) If you've waited on a review for a
> month and pinged people several times, then by all means feel free to
> commit immediately after getting a LGTM.
I feel like something should be spelled out about what to do if no
responses are given to a patch.  I've had that happen multiple times,
pinging once a week for a month or more.  What is the author supposed to
do?

                        -David

On Wed, Nov 20, 2019 at 2:06 PM David Greene via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> > Of course, this is usually not an issue in LLVM today because
we're so
> > bad at reviewing code in general :) If you've waited on a review
for a
> > month and pinged people several times, then by all means feel free to
> > commit immediately after getting a LGTM.
>
> I feel like something should be spelled out about what to do if no
> responses are given to a patch.  I've had that happen multiple times,
> pinging once a week for a month or more.  What is the author supposed to
> do?
>
Basically that - but yeah, it should be written down & maybe an SLA of some
kind of "hey, at least reply to someone and acknowledge it's
there/explain
why review is slow, etc" - but you can also escalate to some extent, by
trying other reviewers, looking for broader code owners if there are any,
etc.

>
>                         -David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191120/57f68200/attachment.html>

On Wed, Nov 20, 2019 at 6:53 PM David Blaikie <dblaikie at gmail.com>
wrote:
> On Wed, Nov 20, 2019 at 2:28 AM Nicolai Hähnle via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
>> On a somewhat related note: Due to timezones, I think good etiquette
>> is to wait a reasonable time before committing a patch if you receive
>> a LGTM very quickly. I think non-trivial patches should generally be
>> up for at least 24 hours -- maybe even extend that to 48 hours --
>> before committing.
>
>
> I'd disagree here - and perhaps my motivation should be enshrined in
the documentation too: I believe a review approval is roughly equivalent to
"I would be comfortable committing this code myself with post-commit
review", so a single approval under those constraints should be able to be
committed immediately & if other reviewers chime in later it can be handled
as it would be with any other post-commit review feedback (which can include
reverting).
Post-commit review should emphatically not be the default. Also, the
social barrier to just unilaterally reverting somebody else's commit
are high, and for a good reason.

Just because two people are comfortable committing code (the author
plus a single reviewer), that does *not* necessarily mean that the
code is good to go in as-is. Consider that both people may be from the
same team, for example. Having a cool-down period is a necessary
antidote against toxic clique behavior.

(This argument may not apply to specific subsystems. For example, if
you're maintaining a small backend that is genuinely only worked on by
a closely-knit team, then it is of course reasonable to work under
more relaxed rules.)

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.