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

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

  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.

  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

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

This all sounds good to me, and reflects certainly how I review and am
reviewed. I don't think I have any additional suggestions.

On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <
llvm-dev at lists.llvm.org> 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).
>
>   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.
>
>   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
>
> --
> 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/20191115/7b5b7f8a/attachment.html>

Do it.
--paulr

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James
Henderson via llvm-dev
Sent: Friday, November 15, 2019 5:17 AM
To: Finkel, Hal J. <hfinkel at anl.gov>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

This all sounds good to me, and reflects certainly how I review and am reviewed.
I don't think I have any additional suggestions.

On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> 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).

  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.

  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

--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

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

+1, more below

On 11/15, 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:
This is great. There is often uncertainty in the review process (I hear
a lot "I cannot review/accept this"), and there are often review
processes that can be improved (I hear complaints about delays and other
things).

>   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).
Fully agreed, especially with 2. in place.

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

>   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.
I like this together with 4.
>   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.
I'd even encourage the second part of this. I would like to see faster
turn-around times by getting good patches in that on which follow-ups
are applied (to some degree at least).

>   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.
This is one that is really important to me. Again, I would go further.
Reviewers that want to be blocking reviewers should look at the whole
patch, to the degree feasible, and not only certain area without leaving
a reason. It is not healthy if we get 10 review rounds with two minor
comments each.

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

> 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.
Yes, please.

> Please let me know what you think.
We should definitively improve our documentation, maybe with links to
outside sources or based on them. I read [0] recently which has a lot of
really good points. Now I am actively trying to improve my reviews based
on that.

[0] https://google.github.io/eng-practices/review/reviewer/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191115/2819289d/attachment-0001.sig>

This looks really great to me Hal, thank you for improving this critical aspect
of the community processes!

-Chris
> On Nov 14, 2019, at 7:46 PM, Finkel, Hal J. via llvm-dev <llvm-dev at
lists.llvm.org> 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).
> 
>  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.
> 
>  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
> 
> -- 
> 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

Sounds generally accurate & good to me.

On Fri, Nov 15, 2019 at 2:17 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> This all sounds good to me, and reflects certainly how I review and am
> reviewed. I don't think I have any additional suggestions.
>
> On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <
> llvm-dev at lists.llvm.org> 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).
>>
>>   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.
>>
>>   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
>>
>> --
>> 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
>>
> _______________________________________________
> 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/20191115/671eca93/attachment-0001.html>

+ 1. Thanks for writing this up Hal.
> On Nov 15, 2019, at 2:16 AM, James Henderson via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> This all sounds good to me, and reflects certainly how I review and am
reviewed. I don't think I have any additional suggestions.
> 
> On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> 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).
> 
>   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.
> 
>   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
> 
> -- 
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
> 
> _______________________________________________
> 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
<https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> _______________________________________________
> 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/20191115/76d31a72/attachment-0001.html>

Thanks for taking the time to update the documentation. This all seems
accurate to me, go for it. :)

On Thu, Nov 14, 2019 at 7:46 PM Finkel, Hal J. via llvm-dev <
llvm-dev at lists.llvm.org> 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).
>
>   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.
>
>   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
>
> --
> 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/20191115/d6538687/attachment-0001.html>

Hi Hal,
I think this is great! I'm happy to help with this if you need a hand.
At the very least I'm happy to be a reviewer :)

Chris Bieneman and I did a tutorial at the dev conference in October on
Contributing to LLVM and we covered many of these points during the
tutorial. It might be good to add a link to that tutorial as part of
this documentation once it's posted. 

A corollary to your point #1 below - if you are not overly familiar with
some area of the compiler your review is still definitely valuable.
However, it's best not to approve the patch as you may not be familiar
with how the patch integrates with the existing code, etc.

Another question that was asked during the tutorial was how to identify
reviewers for your patches, so expanding our documentation on that might
be good also.

Kit

"Finkel, Hal J. via llvm-dev" <llvm-dev at lists.llvm.org>
writes:
> 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).
>
>   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.
>
>   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

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

+1 in general, and Philip has good suggestions as well!

-- 
Mehdi


On Sat, Nov 16, 2019 at 8:37 AM 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/20191116/73ac2884/attachment.html>

>
> 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 Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev
<llvm-dev at lists.llvm.org> 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).
>
>   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.
>
>   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.
+1 to all of this, thank you Hal!

~Aaron
>
> Thanks again,
>
> Hal
>
> --
> 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

+1 from me. Also, FWIW, I tried (but never finished) drafting
something similar but more focused on review in context of Phabricator
a while back (https://reviews.llvm.org/D56482).

On Fri, Nov 15, 2019 at 3:46 AM Finkel, Hal J. via llvm-dev
<llvm-dev at lists.llvm.org> 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).
>
>   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.
>
>   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
>
> --
> 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

On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>   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).
>
I disagree on the high bar here. The author should acknowledge the
comments; however, addressing all of the comments in one shot has similar
problems as having commits that are too large (diffs between revisions
become more difficult to review). This also leads to significant timing
issues, where the comments made overnight in some time zone are addressed
by the author locally, but someone added comments in the afternoon the next
day before the author has a chance to post the new revision.

> If you suggest changes in a code review, but
> don't wish the suggestion to be interpreted this strongly, please state
> so explicitly.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/0b185520/attachment.html>

On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:
> On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>
>       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).
>
> I disagree on the high bar here. The author should acknowledge the 
> comments; however, addressing all of the comments in one shot has 
> similar problems as having commits that are too large (diffs between 
> revisions become more difficult to review). This also leads to 
> significant timing issues, where the comments made overnight in some 
> time zone are addressed by the author locally, but someone added 
> comments in the afternoon the next day before the author has a chance 
> to post the new revision.
Then the author needs to indicate this explicitly, and except in 
exceptional circumstances with an explicit request, should not expect 
re-review until comments are addressed.  It's fine to post a new version 
of the patch; just not to expect action by reviewers.

I see this one as a major stumbling block for new contributors - i.e. 
reviews get stuck because both sides expect the other to be doing 
something.  Having it clearly documented is important IMO.
>     If you suggest changes in a code review, but
>     don't wish the suggestion to be interpreted this strongly, please
>     state
>     so explicitly.
>
>
> _______________________________________________
> 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/20191202/3f7220ce/attachment.html>

On 12/2/19 10:52 AM, Hubert Tong wrote:
On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:
  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).
I disagree on the high bar here. The author should acknowledge the comments;
however, addressing all of the comments in one shot has similar problems as
having commits that are too large (diffs between revisions become more difficult
to review).


I'm happy to use acknowledge instead of address. That's probably closer
to what I intended. It is certainly true that separable enhancements (i.e., not
required for correctness) are normally better as follow up patches, and we
should make sure that's clear.


This also leads to significant timing issues, where the comments made overnight
in some time zone are addressed by the author locally, but someone added
comments in the afternoon the next day before the author has a chance to post
the new revision.


Good point. We should say "in a future revision"; there are a number
of reasons why updates might be staged.

 -Hal



If you suggest changes in a code review, but
don't wish the suggestion to be interpreted this strongly, please state
so explicitly.

--
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/20191203/c049973d/attachment.html>

I would like to thank everyone who provided feedback on this thread. I 
attempted to incorporate all of it. If I missed something, please let me 
know.

I've posted a patch to our documentation here: 
https://reviews.llvm.org/D71916

  -Hal

On 11/14/19 9:45 PM, Hal Finkel 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).
>
>  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.
>
>  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
>
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory