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