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

>
> 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.

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

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").

Thoughts?

 -Hal


James

On Sat, 16 Nov 2019 at 16:37, Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org<mailto: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<mailto: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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191118/d50216d6/attachment.html>

On Mon, Nov 18, 2019 at 7:53 AM 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").
>
Thoughts?
>
Generally sounds good to me though " (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)" - makes me a bit twitchy. I
think it's pretty reasonable for someone to say "LGTM, but also wait
for @someone" without having a week deadline. I do that pretty often -
hoping to provide a first pass review but there being a design decision or
the like that I feel is out of my depth and needs sign off from a code
owner or similar, essentially.

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

All of the variations suggested seem reasonable to me.  I minorly prefer 
James' wording, but any of them are better than nothing.

Philip

On 11/18/19 7:53 AM, Finkel, Hal J. 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").
>
> Thoughts?
>
>  -Hal
>
>
>>
>> James
>>
>> On Sat, 16 Nov 2019 at 16:37, Philip Reames via llvm-dev 
>> <llvm-dev at lists.llvm.org <mailto: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
>>     <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 <mailto: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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191118/876b94d2/attachment.html>

On Mon, 18 Nov 2019 at 15:53, Finkel, Hal J. via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> 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").
>
> Thoughts?
I felt your original proposal was missing this part, but everything
else was fine. So adding this, LGTM. :)

Maybe this paragraph is a bit convoluted and too big, but that's not
important, as long as we pass the information.

One comment on the format, though...

Not everyone reads all paragraphs, but all of them are important. I
have two tactics that make sure people pay attention to what's
necessary:

1. The first sentence of the bullet point is very short, and _conveys
the whole point_.

The paragraph can continue, expanding on the meaning, but the number
of people that will read is reduced, like 1/x for each new word. If
people understand and agree with the sentence, not much else is
needed. If they disagree or don't understand, they can go as far as
they need to make sense of it. An example is my phrase up there, and
this paragraph here. :)

2. Make the first sentence in **bold**.

This helps quick-reading and finding information, especially on long
pages/sections. Further italics or underline of specific core words
can be done, if necessary. Honestly, I could have said "short and
bold" on the first bullet point, but I wanted to have two, to
demonstrate the point, as the first sentece was already too long and
people won't read the word "bold", even if it was in bold.

cheers,
--renato

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.