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