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

classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Mircea Trofin via llvm-dev
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev

Do it.

--paulr

 

From: llvm-dev <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Friday, November 15, 2019 5:17 AM
To: Finkel, Hal J. <[hidden email]>
Cc: [hidden email]
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
+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/

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

signature.asc (235 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
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 <[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
Sounds generally accurate & good to me.

On Fri, Nov 15, 2019 at 2:17 AM James Henderson via llvm-dev <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
+ 1. Thanks for writing this up Hal.

On Nov 15, 2019, at 2:16 AM, James Henderson via llvm-dev <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
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" <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
+ 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
+1, looks good to me.

On Sat, Nov 16, 2019 at 7:12 PM David Blaikie via llvm-dev
<[hidden email]> wrote:

>
> Sounds generally accurate & good to me.
>
> On Fri, Nov 15, 2019 at 2:17 AM James Henderson via llvm-dev <[hidden email]> 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 <[hidden email]> 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
Roman.

>>> _______________________________________________
>>> LLVM Developers mailing list
>>> [hidden email]
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
+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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev

Thanks, everyone! I'll work on a patch soon incorporating the feedback on this thread.

 -Hal

On 11/16/19 7:56 PM, Mehdi AMINI wrote:
+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 <[hidden email]> 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
[hidden email]
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev
On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev
<[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev


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 <[hidden email]> 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
[hidden email]
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev


On Mon, Nov 18, 2019 at 7:53 AM Finkel, Hal J. via llvm-dev <[hidden email]> 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 <[hidden email]> 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
[hidden email]
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via llvm-dev

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 <[hidden email]> 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
[hidden email]
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
One thing I wish our code review policy expressed was the intent of code reviews.

Personally, I believe the intent of code reviews is to improve the quality of code, to avoid the introduction of defects that another contributor could identify, and generally to benefit from the experience of other contributors.

One behavior that runs contrary to that intent (which has occasionally occurred in LLVM), is having your friend LGTM your patches as a rubber stamp. I'm not sure the right way to address that, but a clear statement of intent for code reviews could be a good first step.

-Chris

On Nov 18, 2019, at 9:54 AM, Philip Reames via llvm-dev <[hidden email]> wrote:

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 <[hidden email]> 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
[hidden email]
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

Mircea Trofin via llvm-dev
In reply to this post by Mircea Trofin via 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
<[hidden email]> 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
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
123