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 150Note that, if a reviewer has requested a particular community member to review, 151and after a week that community member has yet to respond, feel free to ping 152the patch (which literally means submitting a comment on the patch with the 153word, "Ping."), or alternatively, ask the original reviewer for further 154suggestions. 155 156If it is likely that others will want to review a recently-posted patch, 157especially if there might be objections, but no one else has done so yet, it is 158also polite to provide a qualified approval (e.g., "LGTM, but please wait for a 159couple of days in case others wish to review"). If approval is received very 160quickly, a patch author may also elect to wait before committing (and this is 161certainly considered polite for non-trivial patches). Especially given the 162global nature of our community, this waiting time should be at least 24 hours. 163Please also be mindful of weekends and major holidays. 164 165Our goal is to ensure community consensus around design decisions and 166significant implementation choices, and one responsibility of a reviewer, when 167providing an overall approval for a patch, is to be reasonably sure that such 168consensus exists. If you're not familiar enough with the community to know, 169then you shouldn't be providing final approval to commit. A reviewer providing 170final approval should have commit access to the LLVM project. 171 172Every patch should be reviewed by at least one technical expert in the areas of 173the project affected by the change. 174 175Splitting Requests and Conditional Acceptance 176--------------------------------------------- 177 178Reviewers may request certain aspects of a patch to be broken out into separate 179patches for independent review. Reviewers may also accept a patch 180conditioned on the author providing a follow-up patch addressing some 181particular issue or concern (although no committed patch should leave the 182project in a broken state). Moreover, reviewers can accept a patch conditioned on 183the author applying some set of minor updates prior to committing, and when 184applicable, it is polite for reviewers to do so. 185 186Don't Unintentionally Block a Review 187------------------------------------ 188 189If you review a patch, but don't intend for the review process to block on your 190approval, please state that explicitly. Out of courtesy, we generally wait on 191committing a patch until all reviewers are satisfied, and if you don't intend 192to look at the patch again in a timely fashion, please communicate that fact in 193the review. 194 195Who Can/Should Review Code? 196=========================== 197 198Non-Experts Should Review Code 199------------------------------ 200 201You do not need to be an expert in some area of the code base to review patches; 202it's fine to ask questions about what some piece of code is doing. If it's not 203clear to you what is going on, you're unlikely to be the only one. Please 204remember that it is not in the long-term best interest of the community to have 205components that are only understood well by a small number of people. Extra 206comments and/or test cases can often help (and asking for comments in the test 207cases is fine as well). 208 209Moreover, authors are encouraged to interpret questions as a reason to reexamine 210the readability of the code in question. Structural changes, or further 211comments, may be appropriate. 212 213If you're new to the LLVM community, you might also find this presentation helpful: 214.. _How to Contribute to LLVM, A 2019 LLVM Developers' Meeting Presentation: https://youtu.be/C5Y977rLqpw 215 216A good way for new contributors to increase their knowledge of the code base is 217to review code. It is perfectly acceptable to review code and explicitly 218defer to others for approval decisions. 219 220Experts Should Review Code 221-------------------------- 222 223If you are an expert in an area of the compiler affected by a proposed patch, 224then you are highly encouraged to review the code. If you are a relevant code 225owner, and no other experts are reviewing a patch, you must either help arrange 226for an expert to review the patch or review it yourself. 227 228Code Reviews, Speed, and Reciprocity 229------------------------------------ 230 231Sometimes code reviews will take longer than you might hope, especially for 232larger features. Common ways to speed up review times for your patches are: 233 234* Review other people's patches. If you help out, everybody will be more 235 willing to do the same for you; goodwill is our currency. 236* Ping the patch. If it is urgent, provide reasons why it is important to you to 237 get this patch landed and ping it every couple of days. If it is 238 not urgent, the common courtesy ping rate is one week. Remember that you're 239 asking for valuable time from other professional developers. 240* Ask for help on IRC. Developers on IRC will be able to either help you 241 directly, or tell you who might be a good reviewer. 242* Split your patch into multiple smaller patches that build on each other. The 243 smaller your patch is, the higher the probability that somebody will take a quick 244 look at it. When doing this, it is helpful to add "[N/M]" (for 1 <= N <= M) to 245 the title of each patch in the series, so it is clear that there is an order 246 and what that order is. 247 248Developers should participate in code reviews as both reviewers and 249authors. If someone is kind enough to review your code, you should return the 250favor for someone else. Note that anyone is welcome to review and give feedback 251on a patch, but approval of patches should be consistent with the policy above. 252