[llvm] 4d0339a - High-Level Code-Review Documentation Update
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 6 20:23:13 PST 2020
Author: Hal Finkel
Date: 2020-03-07T04:20:18Z
New Revision: 4d0339aecb6086048c543ab55c12e835fbe4514e
URL: https://github.com/llvm/llvm-project/commit/4d0339aecb6086048c543ab55c12e835fbe4514e
DIFF: https://github.com/llvm/llvm-project/commit/4d0339aecb6086048c543ab55c12e835fbe4514e.diff
LOG: High-Level Code-Review Documentation Update
This is an update to the documentation of 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. This is, of course, just one further
step in better documenting our community processes. I expect we'll continue to
improve this over time. Thank you to everyone who provided feedback!
Differential Revision: https://reviews.llvm.org/D71916
Added:
llvm/docs/CodeReview.rst
Modified:
llvm/docs/Contributing.rst
llvm/docs/DeveloperPolicy.rst
llvm/docs/Lexicon.rst
Removed:
################################################################################
diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
new file mode 100644
index 000000000000..3350b21a983e
--- /dev/null
+++ b/llvm/docs/CodeReview.rst
@@ -0,0 +1,237 @@
+=====================================
+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.
+ * Help grow and develop new contributors, through mentorship by community leaders.
+
+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 patches to be reviewed before being committed. Smaller patches
+(or patches 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 patch is also
+responsible for making all necessary review-related changes, including
+those 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.
+
+In addition, if substantial problems are identified, it is expected that the
+patch is reverted and fixed offline. Before being recommitted, the patch
+generally undergoes further review, including by the community member who
+identified the problem and, in cases where the patch triggered a
+hardware-specific buildbot failure, a community member with access to hardware
+similar to that on the buildbot that the patch previously caused to fail.
+
+Please note: The bar for post-commit feedback is not higher than for pre-commit
+feedback. Don't delay unnecessarily in providing feedback. However, 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.
+
+That having been said, if a substantial period of time has passed since the
+original change was committed, it may be better to create a new patch to
+address the issues than comment on the original commit. The original patch
+author, for example, might no longer be an active contributor to the project.
+
+What Tools Are Used for Code Review?
+------------------------------------
+
+Code reviews are conducted, in order of preference, on our web-based
+code-review tool (see :doc:`Phabricator`), by email on the relevant project's
+commit mailing list, on the project's development list, or on the 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
+(Request for Comment) email on the project's ``*-dev`` mailing list first. For
+changes that promise significant impact on users and/or downstream code bases,
+reviewers can request an RFC 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 when providing the updated patch. When using the web-based code-review
+tool, such notes can be provided in the "Diff" description (which is
diff erent
+from the description of the "Differential Revision" as a whole used for the
+commit message).
+
+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
+the patch (which literally means submitting a comment on the patch with the
+word, "Ping."), 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 of 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.
+Please also be mindful of weekends and major holidays.
+
+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. A reviewer providing
+final approval should have commit access to the LLVM project.
+
+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. Reviewers may also 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). Moreover, reviewers can 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 code base 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. 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. Common 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 is, the higher the probability that somebody will take a quick
+ look at it. When doing this, it is helpful to add "[N/M]" (for 1 <= N <= M) to
+ the title of each patch in the series, so it is clear that there is an order
+ and what that order is.
+
+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 approval of patches should be consistent with the policy above.
diff --git a/llvm/docs/Contributing.rst b/llvm/docs/Contributing.rst
index 2ad0d9080e12..9185bfdf4917 100644
--- a/llvm/docs/Contributing.rst
+++ b/llvm/docs/Contributing.rst
@@ -110,6 +110,8 @@ 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
==============================
diff --git a/llvm/docs/DeveloperPolicy.rst b/llvm/docs/DeveloperPolicy.rst
index e38ce93217b3..0b3051a4393b 100644
--- a/llvm/docs/DeveloperPolicy.rst
+++ b/llvm/docs/DeveloperPolicy.rst
@@ -121,49 +121,9 @@ licensing terms and may result in your contribution being excluded.
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 GitHub commit 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:
@@ -353,10 +313,9 @@ This is normal and will be done when the mailing list owner has time.
If you have recently been granted commit access, these policies apply:
-#. You are granted *commit-after-approval* to all parts of LLVM. To get
- approval, submit a `patch`_ to `llvm-commits
- <http://lists.llvm.org/mailman/listinfo/llvm-commits>`_. When approved,
- you may commit it yourself.
+#. You are granted *commit-after-approval* to all parts of LLVM. For
+ information on how to get approval for a patch, please see :doc:`CodeReview`.
+ When approved, you may commit it yourself.
#. You are allowed to commit patches without approval which you think are
obvious. This is clearly a subjective decision --- we simply expect you to
diff --git a/llvm/docs/Lexicon.rst b/llvm/docs/Lexicon.rst
index d42ce90730f1..b0a6e4655fe8 100644
--- a/llvm/docs/Lexicon.rst
+++ b/llvm/docs/Lexicon.rst
@@ -230,6 +230,10 @@ R
and other optimization. For example, changing ``(A+B-A)`` into ``(B+A-A)``,
permitting it to be optimized into ``(B+0)`` then ``(B)``.
+**RFC**
+ Request for Comment. An email sent to a project mailing list in order to
+ solicit feedback on a proposed change.
+
.. _roots:
.. _stack roots:
More information about the llvm-commits
mailing list