From 4d0339aecb6086048c543ab55c12e835fbe4514e Mon Sep 17 00:00:00 2001 From: Hal Finkel Date: Thu, 26 Dec 2019 21:56:26 +0000 Subject: [PATCH] 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 --- llvm/docs/CodeReview.rst | 237 ++++++++++++++++++++++++++++++++++ llvm/docs/Contributing.rst | 2 + llvm/docs/DeveloperPolicy.rst | 53 +------- llvm/docs/Lexicon.rst | 4 + 4 files changed, 249 insertions(+), 47 deletions(-) create mode 100644 llvm/docs/CodeReview.rst 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 different +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 - `_. 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: