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