High-Level Code-Review Documentation Update

[hfinkel]

Summary

An update to the documentation on our community code-review process. Based on the RFC: High-Level Code-Review Documentation Update (http://lists.llvm.org/pipermail/llvm-dev/2019-November/136808.html).

In this patch, I've pulled out the documentation into a separate file, and broken it into a number of subsections. I'm certainly open to feedback on both structure and content.

[jdoerfert]

Thanks again for doing this! As a general suggestion for code owners, "experts in areas", and people interested, I'd mention Herald rules to get notified if a patch touches a file or contains a keyword.

[lattner]

This looks really great to me, thank you for driving this! I'd recommend giving this a couple of days for others to comment (as the doc itself suggest :-), given the global nature of the change. Thanks!

llvm/docs/CodeReview.rst

10

I'd recommend adding a fourth bullet, along the lines of:

• Help grow and develop new contributors, through mentorship by community leaders.

122

Recommend s/him or her/them/ to be inclusive of our nonbinary friends.

[mehdi_amini]

LGTM (but wait for the others contributors to the email threads, especially during this holidays season).

llvm/docs/CodeReview.rst

207

"Ask for help on IRC or Discord."?

[jdoerfert]

Just to make my approval formal: LGTM.

[MaskRay]

llvm/docs/CodeReview.rst

48

The bar for post-commit feedback is not higher than pre-commit feedback ?

67

``*-dev``

207

From https://lists.llvm.org/pipermail/llvm-dev/2019-November/136880.html , people are concerned about Discord's user policy. Not mentioning it here (before we reach a consensus) should be fine.

210

smaller your patch is

217

Delete the trailing empty line.

[hfinkel]

llvm/docs/CodeReview.rst

48

Do you disagree? This is an important question. I've never felt that there is a higher bar, but I've certainly heard people say that they feel otherwise in practice. I don't think that's optimal - if someone has feedback, we should hear it, and they shouldn't feel barred by the fact that the commit has happened already.

Clearly there a practical timing issue: if I comment on a patch after the committer has stopped working on the project, I might not receive any reply, and I'll really just need to submit a patch myself. If there are additional distinctions that we wish to capture, I'm certainly happy to try to do so.

[aaron.ballman]

LGTM!

llvm/docs/CodeReview.rst

48

I think I read what @MaskRay wrote as a suggestion to improve the wording while keeping the same intent. The current phrasing is a bit awkward and I think the proposed wording is a bit better.

(FWIW, I agree that the bars are the same for pre- and post-commit feedback.)

131

Should we also mention something about keeping weekends and holidays in mind?

[MaskRay]

llvm/docs/CodeReview.rst

48

Sorry if I was unclear. I totally agree with this ("the bars are the same for pre- and post-commit feedback."). Just suggested an alternative wording..

[modocache]

Thanks for this! This LGTM but I have two non-blocking suggestions.

llvm/docs/CodeReview.rst

67

New contributors may not be able to glean that *-dev refers to one of a number of mailing lists. May I suggest changing this sentence slightly, to: "...and so on, require a "request for comments" (RFC) email to one of the LLVM project's *-dev mailing lists first."

68

I really appreciate spelling out the "RFC" acronym here, since brand-new contributors may not be familiar with this term. If I could make one or two small suggestions:

1. "RFC" first appears in the sentence before this one, in "require an RFC on *-dev first." Maybe the parenthetical "(Request for Comment)" should appear there instead of in this sentence?
2. "RFC" doesn't appear in the LLVM lexicon. Maybe you could add it, either as part of this patch, or in a separate commit?

[Mordante]

Thanks for writing this document! I noticed the document contains a lot of long sentences. These may be harder to understand for non-native speakers. I pointed a few out, but for some I don't have better suggestions.

llvm/docs/CodeReview.rst

28

Maybe change committed, but smaller to committed. Smaller in order to improve the readability?

35

Maybe change changes, and these to changes. These in order to improve the readability?

49

Maybe change feedback, but if to feedback. If in order to improve the readability?

134

Maybe change choices, and one to choices. One in order to improve the readability?

146

Maybe change review, and also, reviewers to review. Reviewers in order to improve the readability?

184

Maybe change code, and it to code. It in order to improve the readability?

207

Add a link to the IRC channel?

215

There are two space here else. Note

[jhenderson]

llvm/docs/CodeReview.rst

51–52

I don't know whether this is needed, but it might be worth noting that if a substantial period of time has passed since the original commit was made, it may be better to create a new patch to address the issues than comment on the original commit.

58–59

What is meant by "development list" here? Also, it's not clear if these three methods are considered equally acceptable at this point, or whether one should be preferred over the other for any given situation.

99

e.g., -> e.g.

Same goes for all other instances of this pattern. Alternatively, consider using "for example,"

101

most-efficient -> most efficient

128

couple days -> couple of days

168

compiler -> code base
or similar - the LLVM project is now much more than a compiler :)

210

Actually, I believe this phrasing is perfectly reasonable English. That being said, if the lack of "is" is confusing for a non-native speaker, there's nothing wrong with it.

[rengolin]

Some inline comments, but otherwise, LGTM. Thanks!

llvm/docs/CodeReview.rst

46

We want to avoid the pattern where a developer uses the buildbots as a trial and error strategy. This is particularly troublesome for slower builders (test-suite, multi-stage, slower/saturated builders).

I would add a recommendation that any patch that has failed in a particular area, and had a member of the community identify and revert, to be reviewed by such member (or the code owner) before it is recommitted.

In particular, I would like this recommendation to be even stronger when the patch doesn't change between iterations.

This means the author is relying on flaky explanations for recommitting and may not be considering that their patch could be the one creating the flakyness. I have seen this happening which had repercussions months later and it was a lot harder to fix the overall problem.

199

s/Accepted/Common/

210

I'd also recommend adding "[N/M]" (for 1 < N < M) on the series, so it is clear that there is an order and what the order is.

Phab is not "phab" in making the order explicit.

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

99

The use with the comma is consistent with the use and description in the MLA Handbook, 6th ed. My understanding is that the comma is customary in North American usage.

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

18

"Be" should be capitalized (as it is a few lines below).

35

I find changes, including those requested to be more readable.

59

The "alternatively" seems to be redundant (and hinders readability IMO).

61

"Is" should be capitalized.

88

"Patch description" is ambiguous between the description of the "Differential revision" and a specific "diff". Depending on whether the author intends to address the feedback in a later commit or just a later update for the current review, one or the other is appropriate.

104

"Is" should be capitalized.

134

The resulting second sentence (with "such consensus") refers to the first. The breaking of the flow by splitting the sentence reduces readability for me.

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

146

If breaking the sentence, then please retain "also": review. Reviewers may also.

149

For variety, I would suggest using something other than "also".

216

I believe this should be the adverb, "consistently", applying to "approved" as a verb. Alternatively: , but approval of patches should be consistent with the policy above.

llvm/docs/DeveloperPolicy.rst

163

Did the "write access"/"commit access" requirement survive?

[hfinkel]

I would like to thank everyone who provided feedback on this patch. I apologize for the slow turn-around time on this. I'll upload a revised version that, I believe, addresses all feedback.

llvm/docs/CodeReview.rst

99

That's correct. This is, unfortunately, a traditional style difference between different parts of the world.

207

It's on the front page along with the mailing lists. If we're going to have links to the channel, I'd rather have a page on it so we can centralize that information (instead of duplicating it in various places throughout the documentation).

llvm/docs/DeveloperPolicy.rst

163

Thanks. I added it back in the LGTM section.

[hfinkel]

Updated per review feedback. Please let me know if you see anything else that should be addressed pre-commit.

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

217

I am still seeing the empty line.

231

Is this meant to imply that the first and the final patches in a series should use a different convention?

[hfinkel]

llvm/docs/CodeReview.rst

231

No. Thanks for catching that.

[hfinkel]

Updated to remove blank line and fix that (1 < N < M) should be (1 <= N <= M).

[hubert.reinterpretcast]

LGTM with one additional comment. Sorry for missing it earlier.

llvm/docs/Lexicon.rst

234 ↗

(On Diff #247459)

I am not very familiar with the format of the file, but it seems the new entry occurs between an entry for "Root" and some way of associating reference labels to that entry.

[hubert.reinterpretcast]

llvm/docs/CodeReview.rst

217

Hmm, it seems we hit one of the situations where Phabricator causes confusion. The empty line that @MaskRay requested removal of (https://reviews.llvm.org/D71916?id=235378#inline-651131) is the one at the end of the file (which is still present now). I am not sure if the empty line which did get removed is necessary in this document format.

[jhenderson]

llvm/docs/CodeReview.rst

231

Maybe:
what the order is -> what that order is

llvm/docs/Lexicon.rst

234 ↗

(On Diff #247459)

Yes, those look like link anchors to me, and probably belong with the **Root** block below.

237 ↗

(On Diff #247459)

Not sure if we have code-style guidelines for docs, but the 80 character limit looks like it is being applied here still, so this needs reflowing.

FWIW, I spell it out as "Request for Comment" in my mind! Don't think it matters though.

[baziotis]

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

[hfinkel]

In D71916#1904329, @baziotis wrote:

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

Thanks! - I've made a minimal update to this section now. I think it would be best to handle more movement there between documents in a separate patch.

[hfinkel]

Update formatting, etc., and add a reference in the new-contributor section of the dev policy in response to reviewer feedback.

[baziotis]

In D71916#1906918, @hfinkel wrote:

In D71916#1904329, @baziotis wrote:

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

Thanks! - I've made a minimal update to this section now. I think it would be best to handle more movement there between documents in a separate patch.

Thanks, great. I'll wait for this to be committed. :)

[jhenderson]

LGTM. Thanks!

[hubert.reinterpretcast]

In D71916#1907203, @jhenderson wrote:

LGTM. Thanks!

LGTM as well.

llvm/docs/CodeReview.rst

+=====================================
+LLVM Code-Review Policy and Practices
+=====================================
+
+LLVM's code-review policy and practices help maintain high code quality across
+the project. Specifically, our code review process aims to:
+
+ * Improve readability and maintainability.
+ * Improve robustness and prevent the introduction of defects.
+ * Best leverage the experience of other contributors for each proposed change.
+
+It is important for all contributors to understand our code-review
+practices and participate in the code-review process.
+
+General Policies
+================
+
+What Code Should be Reviewed?
+-----------------------------
+
+All developers are required to have significant changes reviewed before they
+are committed to the repository.
+
+Must Code Be Reviewed Prior to Being Committed?
+-----------------------------------------------
+
+Code can be reviewed either before it is committed or after. We expect
+significant changes to be reviewed before being committed, but smaller changes
+(or changes where the developer owns the component) that meet
+likely-community-consensus requirements (as apply to all patch approvals) can
+be committed prior to an explicit review. In situations where there is any
+uncertainty, a patch should be reviewed prior to being committed.
+
+Please note that the developer responsible for a code change is also
+responsible for making all necessary review-related changes, and these include
+changes requested during any post-commit review.
+
+Can Code Be Reviewed After It Is Committed?
+-------------------------------------------
+
+Post-commit review is encouraged, and can be accomplished using any of the
+tools detailed below. 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).
+
+Please note: There is no bar for post-commit feedback that is higher than for
+pre-commit feedback. Don't delay unnecessarily in providing feedback, but if
+you see something after code has been committed about which you would have
+commented pre-commit (had you noticed it earlier), please feel free to provide
+that feedback at any time.
+
+What Tools Are Used for Code Review?
+------------------------------------
+
+Code reviews are conducted on our web-based code-review tool (see
+:doc:`Phabricator`), by email on the relevant project's commit mailing list, or
+alternatively on the project's development list or bug tracker.
+
+When is an RFC Required?
+------------------------
+
+Some changes are too significant for just a code review. Changes that should
+change the LLVM 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.
+
+Code-Review Workflow
+====================
+
+Code review can be an iterative process, which continues until the patch is
+ready to be committed. Specifically, once a patch is sent out for review, it
+needs an explicit approval before it is committed. Do not assume silent
+approval, or solicit objections to a patch with a deadline.
+
+Acknowledge All Reviewer Feedback
+---------------------------------
+
+All comments by reviewers should be acknowledged by the patch author. It is
+generally expected that suggested changes will be incorporated into a future
+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 a new patch
+does not address all outstanding feedback, the author should explicitly state
+that in the patch description. If you suggest changes in a code review, but
+don't wish the suggestion to be interpreted this strongly, please state so
+explicitly.
+
+Aim to Make Efficient Use of Everyone's Time
+--------------------------------------------
+
+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 and reviewers to make the most-efficient
+use of their time.
+
+LGTM - How a Patch is Accepted
+------------------------------
+
+A patch is approved to be committed when a reviewer accepts it, and this is
+almost always associated with a message containing the text "LGTM" (which
+stands for Looks Good To Me). Only approval from a single reviewer is required.
+
+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.
+
+Note that, if a reviewer has requested a particular community member to review,
+and after a week that community member has yet to respond, feel free to ping
+him or her, or alternatively, ask the original reviewer for further
+suggestions.
+
+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"). If approval is received very
+quickly, a patch author may also elect to wait before committing (and this is
+certainly considered polite for non-trivial patches). Especially given the
+global nature of our community, this waiting time should be at least 24 hours.
+
+Our goal is to ensure community consensus around design decisions and
+significant implementation choices, and one responsibility of a reviewer, when
+providing an overall approval for a patch, is to be reasonably sure that such
+consensus exists. If you're not familiar enough with the community to know,
+then you shouldn't be providing final approval to commit.
+
+Every patch should be reviewed by at least one technical expert in the areas of
+the project affected by the change.
+
+Splitting Requests and Conditional Acceptance
+---------------------------------------------
+
+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.
+
+Don't Unintentionally Block a Review
+------------------------------------
+
+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.
+
+Who Can/Should Review Code?
+===========================
+
+Non-Experts Should Review Code
+------------------------------
+
+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. Please
+remember that it is not in the long-term best interest of the community to have
+components that are only understood well by a small number of people. Extra
+comments and/or test cases can often help (and asking for comments in the test
+cases is fine as well).
+
+Moreover, authors are encouraged to interpret questions as a reason to reexamine
+the readability of the code in question. Structural changes, or further
+comments, may be appropriate.
+
+If you're new to the LLVM community, you might also find this presentation helpful:
+.. _How to Contribute to LLVM, A 2019 LLVM Developers' Meeting Presentation: https://youtu.be/C5Y977rLqpw
+
+A good way for new contributors to increase their knowledge of the code base is
+to review code, and it is perfectly acceptable to review code and explicitly
+defer to others for approval decisions.
+
+Experts Should Review Code
+--------------------------
+
+If you are an expert in an area of the compiler affected by a proposed patch,
+then you are highly encouraged to review the code. If you are a relevant code
+owner, and no other experts are reviewing a patch, you must either help arrange
+for an expert to review the patch or review it yourself.
+
+Code Reviews, Speed, and Reciprocity
+------------------------------------
+
+Sometimes code reviews will take longer than you might hope, especially for
+larger features. Accepted ways to speed up review times for your patches are:
+
+* Review other people's patches. If you help out, everybody will be more
+  willing to do the same for you; goodwill is our currency.
+* Ping the patch. If it is urgent, provide reasons why it is important to you to
+  get this patch landed and ping it every couple of days. If it is
+  not urgent, the common courtesy ping rate is one week. Remember that you're
+  asking for valuable time from other professional developers.
+* Ask for help on IRC. Developers on IRC will be able to either help you
+  directly, or tell you who might be a good reviewer.
+* Split your patch into multiple smaller patches that build on each other. The
+  smaller your patch, the higher the probability that somebody will take a quick
+  look at it.
+
+Developers should participate in code reviews as both reviewers and
+authors. If someone is kind enough to review your code, you should return the
+favor for someone else.  Note that anyone is welcome to review and give feedback
+on a patch, but patches should be approved only consistent with the policy above.
+

llvm/docs/Contributing.rst

 on your behalf.
 
 If you have received no comments on your patch for a week, you can request a
 review by 'ping'ing a patch by responding to the email thread containing the
 patch, or the Phabricator review with "Ping." The common courtesy 'ping' rate
 is once a week. Please remember that you are asking for valuable time from other
 professional developers.
 
+For more information on LLVM's code-review process, please see :doc:`CodeReview`.
+
 
 Helpful Information About LLVM
 ==============================
 :doc:`LLVM's documentation <index>` provides a wealth of information about LLVM's internals as
 well as various user guides. The pages listed below should provide a good overview
 of LLVM's high-level design, as well as its internals:
 
 :doc:`GettingStarted`

llvm/docs/DeveloperPolicy.rst

 notices to the patches themselves.  These notices conflict with the LLVM
 licensing terms and may result in your contribution being excluded.
 
 .. _code review:
 
 Code Reviews
 ------------
 
-LLVM has a code review policy. Code review is one way to increase the quality of
-software. We generally follow these policies:
-
-#. All developers are required to have significant changes reviewed before they
-   are committed to the repository.
-
-#. 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.
-
-#. Code can be reviewed either before it is committed or after.  We expect major
-   changes to be reviewed before being committed, but smaller changes (or
-   changes where the developer owns the component) can be reviewed after commit.
-
-#. The developer responsible for a code change is also responsible for making
-   all necessary review-related changes.
-
-#. Code review can be an iterative process, which continues until the patch is
-   ready to be committed. Specifically, once a patch is sent out for review, it
-   needs an explicit "looks good" before it is submitted. Do not assume silent
-   approval, or request active objections to the patch with a deadline.
-
-Sometimes code reviews will take longer than you would hope for, especially for
-larger features. Accepted ways to speed up review times for your patches are:
-
-* Review other people's patches. If you help out, everybody will be more
-  willing to do the same for you; goodwill is our currency.
-* Ping the patch. If it is urgent, provide reasons why it is important to you to
-  get this patch landed and ping it every couple of days. If it is
-  not urgent, the common courtesy ping rate is one week. Remember that you're
-  asking for valuable time from other professional developers.
-* Ask for help on IRC. Developers on IRC will be able to either help you
-  directly, or tell you who might be a good reviewer.
-* Split your patch into multiple smaller patches that build on each other. The
-  smaller your patch, the higher the probability that somebody will take a quick
-  look at it.
-
-Developers should participate in code reviews as both reviewers and
-reviewees. If someone is kind enough to review your code, you should return the
-favor for someone else.  Note that anyone is welcome to review and give feedback
-on a patch, but only people with Subversion write access can approve it.
-
-There is a web based code review tool that can optionally be used
-for code reviews. See :doc:`Phabricator`.
+LLVM has a code-review policy. Code review is one way to increase the quality of
+software. Please see :doc:`CodeReview` for more information on LLVM's code-review
+process.
 
 .. _code owners:
 
 Code Owners
 -----------
 
 The LLVM Project relies on two features of its process to maintain rapid
 development in addition to the high quality of its source base: the combination