1===================================== 2LLVM Code-Review Policy and Practices 3===================================== 4 5LLVM's code-review policy and practices help maintain high code quality across 6the project. Specifically, our code review process aims to: 7 8 * Improve readability and maintainability. 9 * Improve robustness and prevent the introduction of defects. 10 * Best leverage the experience of other contributors for each proposed change. 11 * Help grow and develop new contributors, through mentorship by community leaders. 12 13It is important for all contributors to understand our code-review 14practices and participate in the code-review process. 15 16General Policies 17================ 18 19What Code Should Be Reviewed? 20----------------------------- 21 22All developers are required to have significant changes reviewed before they 23are committed to the repository. 24 25Must Code Be Reviewed Prior to Being Committed? 26----------------------------------------------- 27 28Code can be reviewed either before it is committed or after. We expect 29significant patches to be reviewed before being committed. Smaller patches 30(or patches where the developer owns the component) that meet 31likely-community-consensus requirements (as apply to all patch approvals) can 32be committed prior to an explicit review. In situations where there is any 33uncertainty, a patch should be reviewed prior to being committed. 34 35Please note that the developer responsible for a patch is also 36responsible for making all necessary review-related changes, including 37those requested during any post-commit review. 38 39Can Code Be Reviewed After It Is Committed? 40------------------------------------------- 41 42Post-commit review is encouraged, and can be accomplished using any of the 43tools detailed below. There is a strong expectation that authors respond 44promptly to post-commit feedback and address it. Failure to do so is cause for 45the patch to be reverted. 46 47In addition, if substantial problems are identified, it is expected that the 48patch is reverted and fixed offline. Before being recommitted, the patch 49generally undergoes further review, including by the community member who 50identified the problem and, in cases where the patch triggered a 51hardware-specific buildbot failure, a community member with access to hardware 52similar to that on the buildbot that the patch previously caused to fail. 53 54Please note: The bar for post-commit feedback is not higher than for pre-commit 55feedback. Don't delay unnecessarily in providing feedback. However, if you see 56something after code has been committed about which you would have commented 57pre-commit (had you noticed it earlier), please feel free to provide that 58feedback at any time. 59 60That having been said, if a substantial period of time has passed since the 61original change was committed, it may be better to create a new patch to 62address the issues than comment on the original commit. The original patch 63author, for example, might no longer be an active contributor to the project. 64 65What Tools Are Used for Code Review? 66------------------------------------ 67 68Code reviews are conducted, in order of preference, on our web-based 69code-review tool (see :doc:`Phabricator`), by email on the relevant project's 70commit mailing list, on the project's development list, or on the bug tracker. 71 72When Is an RFC Required? 73------------------------ 74 75Some changes are too significant for just a code review. Changes that should 76change the LLVM Language Reference (e.g., adding new target-independent 77intrinsics), adding language extensions in Clang, and so on, require an RFC 78(Request for Comment) email on the project's ``*-dev`` mailing list first. For 79changes that promise significant impact on users and/or downstream code bases, 80reviewers can request an RFC achieving consensus before proceeding with code 81review. That having been said, posting initial patches can help with 82discussions on an RFC. 83 84Code-Review Workflow 85==================== 86 87Code review can be an iterative process, which continues until the patch is 88ready to be committed. Specifically, once a patch is sent out for review, it 89needs an explicit approval before it is committed. Do not assume silent 90approval, or solicit objections to a patch with a deadline. 91 92Acknowledge All Reviewer Feedback 93--------------------------------- 94 95All comments by reviewers should be acknowledged by the patch author. It is 96generally expected that suggested changes will be incorporated into a future 97revision of the patch unless the author and/or other reviewers can articulate a 98good reason to do otherwise (and then the reviewers must agree). If a new patch 99does not address all outstanding feedback, the author should explicitly state 100that when providing the updated patch. When using the web-based code-review 101tool, such notes can be provided in the "Diff" description (which is different 102from the description of the "Differential Revision" as a whole used for the 103commit message). 104 105If you suggest changes in a code review, but don't wish the suggestion to be 106interpreted this strongly, please state so explicitly. 107 108Aim to Make Efficient Use of Everyone's Time 109-------------------------------------------- 110 111Aim to limit the number of iterations in the review process. For example, when 112suggesting a change, if you want the author to make a similar set of changes at 113other places in the code, please explain the requested set of changes so that 114the author can make all of the changes at once. If a patch will require 115multiple steps prior to approval (e.g., splitting, refactoring, posting data 116from specific performance tests), please explain as many of these up front as 117possible. This allows the patch author and reviewers to make the most efficient 118use of their time. 119 120LGTM - How a Patch Is Accepted 121------------------------------ 122 123A patch is approved to be committed when a reviewer accepts it, and this is 124almost always associated with a message containing the text "LGTM" (which 125stands for Looks Good To Me). Only approval from a single reviewer is required. 126 127When providing an unqualified LGTM (approval to commit), it is the 128responsibility of the reviewer to have reviewed all of the discussion and 129feedback from all reviewers ensuring that all feedback has been addressed and 130that all other reviewers will almost surely be satisfied with the patch being 131approved. If unsure, the reviewer should provide a qualified approval, (e.g., 132"LGTM, but please wait for @someone, @someone_else"). You may also do this if 133you are fairly certain that a particular community member will wish to review, 134even if that person hasn't done so yet. 135 136Note that, if a reviewer has requested a particular community member to review, 137and after a week that community member has yet to respond, feel free to ping 138the patch (which literally means submitting a comment on the patch with the 139word, "Ping."), or alternatively, ask the original reviewer for further 140suggestions. 141 142If it is likely that others will want to review a recently-posted patch, 143especially if there might be objections, but no one else has done so yet, it is 144also polite to provide a qualified approval (e.g., "LGTM, but please wait for a 145couple of days in case others wish to review"). If approval is received very 146quickly, a patch author may also elect to wait before committing (and this is 147certainly considered polite for non-trivial patches). Especially given the 148global nature of our community, this waiting time should be at least 24 hours. 149Please also be mindful of weekends and major holidays. 150 151Our goal is to ensure community consensus around design decisions and 152significant implementation choices, and one responsibility of a reviewer, when 153providing an overall approval for a patch, is to be reasonably sure that such 154consensus exists. If you're not familiar enough with the community to know, 155then you shouldn't be providing final approval to commit. A reviewer providing 156final approval should have commit access to the LLVM project. 157 158Every patch should be reviewed by at least one technical expert in the areas of 159the project affected by the change. 160 161Splitting Requests and Conditional Acceptance 162--------------------------------------------- 163 164Reviewers may request certain aspects of a patch to be broken out into separate 165patches for independent review. Reviewers may also accept a patch 166conditioned on the author providing a follow-up patch addressing some 167particular issue or concern (although no committed patch should leave the 168project in a broken state). Moreover, reviewers can accept a patch conditioned on 169the author applying some set of minor updates prior to committing, and when 170applicable, it is polite for reviewers to do so. 171 172Don't Unintentionally Block a Review 173------------------------------------ 174 175If you review a patch, but don't intend for the review process to block on your 176approval, please state that explicitly. Out of courtesy, we generally wait on 177committing a patch until all reviewers are satisfied, and if you don't intend 178to look at the patch again in a timely fashion, please communicate that fact in 179the review. 180 181Who Can/Should Review Code? 182=========================== 183 184Non-Experts Should Review Code 185------------------------------ 186 187You do not need to be an expert in some area of the code base to review patches; 188it's fine to ask questions about what some piece of code is doing. If it's not 189clear to you what is going on, you're unlikely to be the only one. Please 190remember that it is not in the long-term best interest of the community to have 191components that are only understood well by a small number of people. Extra 192comments and/or test cases can often help (and asking for comments in the test 193cases is fine as well). 194 195Moreover, authors are encouraged to interpret questions as a reason to reexamine 196the readability of the code in question. Structural changes, or further 197comments, may be appropriate. 198 199If you're new to the LLVM community, you might also find this presentation helpful: 200.. _How to Contribute to LLVM, A 2019 LLVM Developers' Meeting Presentation: https://youtu.be/C5Y977rLqpw 201 202A good way for new contributors to increase their knowledge of the code base is 203to review code. It is perfectly acceptable to review code and explicitly 204defer to others for approval decisions. 205 206Experts Should Review Code 207-------------------------- 208 209If you are an expert in an area of the compiler affected by a proposed patch, 210then you are highly encouraged to review the code. If you are a relevant code 211owner, and no other experts are reviewing a patch, you must either help arrange 212for an expert to review the patch or review it yourself. 213 214Code Reviews, Speed, and Reciprocity 215------------------------------------ 216 217Sometimes code reviews will take longer than you might hope, especially for 218larger features. Common ways to speed up review times for your patches are: 219 220* Review other people's patches. If you help out, everybody will be more 221 willing to do the same for you; goodwill is our currency. 222* Ping the patch. If it is urgent, provide reasons why it is important to you to 223 get this patch landed and ping it every couple of days. If it is 224 not urgent, the common courtesy ping rate is one week. Remember that you're 225 asking for valuable time from other professional developers. 226* Ask for help on IRC. Developers on IRC will be able to either help you 227 directly, or tell you who might be a good reviewer. 228* Split your patch into multiple smaller patches that build on each other. The 229 smaller your patch is, the higher the probability that somebody will take a quick 230 look at it. When doing this, it is helpful to add "[N/M]" (for 1 <= N <= M) to 231 the title of each patch in the series, so it is clear that there is an order 232 and what that order is. 233 234Developers should participate in code reviews as both reviewers and 235authors. If someone is kind enough to review your code, you should return the 236favor for someone else. Note that anyone is welcome to review and give feedback 237on a patch, but approval of patches should be consistent with the policy above. 238