293 lines · plain
1.. _code_review_policy:2 3=====================================4LLVM Code-Review Policy and Practices5=====================================6 7LLVM's code-review policy and practices help maintain high code quality across8the project. Specifically, our code review process aims to:9 10 * Improve readability and maintainability.11 * Improve robustness and prevent the introduction of defects.12 * Best leverage the experience of other contributors for each proposed change.13 * Help grow and develop new contributors, through mentorship by community leaders.14 15It is important for all contributors to understand our code-review16practices and participate in the code-review process.17 18General Policies19================20 21What Code Should Be Reviewed?22-----------------------------23 24All developers are required to have significant changes reviewed before they25are committed to the repository.26 27Must Code Be Reviewed Prior to Being Committed?28-----------------------------------------------29 30Code can be reviewed either before it is committed or after. We expect31significant patches to be reviewed before being committed. Smaller patches32(or patches where the developer owns the component) that meet33likely-community-consensus requirements (as apply to all patch approvals) can34be committed prior to an explicit review. In situations where there is any35uncertainty, a patch should be reviewed prior to being committed.36 37Please note that the developer responsible for a patch is also38responsible for making all necessary review-related changes, including39those requested during any post-commit review.40 41.. _post_commit_review:42 43Can Code Be Reviewed After It Is Committed?44-------------------------------------------45 46Post-commit review is encouraged, and can be accomplished using any of the47tools detailed below. There is a strong expectation that authors respond48promptly to post-commit feedback and address it. Failure to do so is cause for49the patch to be :ref:`reverted <revert_policy>`.50 51If a community member expresses a concern about a recent commit, and this52concern would have been significant enough to warrant a conversation during53pre-commit review (including around the need for more design discussions),54they may ask for a revert to the original author who is responsible to revert55the patch promptly. Developers often disagree, and erring on the side of the56developer asking for more review prevents any lingering disagreement over57code in the tree. This does not indicate any fault from the patch author,58this is inherent to our post-commit review practices.59Reverting a patch ensures that design discussions can happen without blocking60other development; it's entirely possible the patch will end up being reapplied61essentially as-is once concerns have been resolved.62 63Before being recommitted, the patch generally should undergo further review.64The community member who identified the problem is expected to engage65actively in the review. In cases where the problem is identified by a buildbot,66a community member with access to hardware similar to that on the buildbot is67expected to engage in the review.68 69Please note: The bar for post-commit feedback is not higher than for pre-commit70feedback. Don't delay unnecessarily in providing feedback. However, if you see71something after code has been committed about which you would have commented72pre-commit (had you noticed it earlier), please feel free to provide that73feedback at any time.74 75That having been said, if a substantial period of time has passed since the76original change was committed, it may be better to create a new patch to77address the issues than comment on the original commit. The original patch78author, for example, might no longer be an active contributor to the project.79 80What Tools Are Used for Code Review?81------------------------------------82 83Pre-commit code reviews are conducted on GitHub with Pull Requests. See84:ref:`GitHub <github-reviews>` documentation.85 86When Is an RFC Required?87------------------------88 89Some changes are too significant for just a code review. Changes that should90change the LLVM Language Reference (e.g., adding new target-independent91intrinsics), adding language extensions in Clang, and so on, require an RFC92(Request for Comment) topic on the `LLVM Discussion Forums <https://discourse.llvm.org>`_93first. For changes that promise significant impact on users and/or downstream94code bases, reviewers can request an RFC achieving consensus before proceeding95with code review. That having been said, posting initial patches can help with96discussions on an RFC.97 98Code-Review Workflow99====================100 101Code review can be an iterative process, which continues until the patch is102ready to be committed. Specifically, once a patch is sent out for review, it103needs an explicit approval before it is committed. Do not assume silent104approval, or solicit objections to a patch with a deadline.105 106.. note::107 If you are using a Pull Request for purposes other than review108 (eg: precommit CI results, convenient web-based reverts, etc)109 `skip-precommit-approval <https://github.com/llvm/llvm-project/labels?q=skip-precommit-approval>`_110 label to the PR.111 112Acknowledge All Reviewer Feedback113---------------------------------114 115All comments by reviewers should be acknowledged by the patch author. It is116generally expected that suggested changes will be incorporated into a future117revision of the patch unless the author and/or other reviewers can articulate a118good reason to do otherwise (and then the reviewers must agree). If a new patch119does not address all outstanding feedback, the author should explicitly state120that when providing the updated patch. When using the web-based code-review121tool, such notes can be provided in the "Diff" description (which is different122from the description of the "Differential Revision" as a whole used for the123commit message).124 125If you suggest changes in a code review, but don't wish the suggestion to be126interpreted this strongly, please state so explicitly.127 128.. note::129 After responding to reviewer comments,130 press `Re-request review <https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/requesting-a-pull-request-review#:~:text=After%20your%20pull%20request%20is%20reviewed>`_131 to bring the Pull Request to the reviewers' attention.132 133Aim to Make Efficient Use of Everyone's Time134--------------------------------------------135 136Aim to limit the number of iterations in the review process. For example, when137suggesting a change, if you want the author to make a similar set of changes at138other places in the code, please explain the requested set of changes so that139the author can make all of the changes at once. If a patch will require140multiple steps prior to approval (e.g., splitting, refactoring, posting data141from specific performance tests), please explain as many of these up front as142possible. This allows the patch author and reviewers to make the most efficient143use of their time.144 145.. _lgtm_how_a_patch_is_accepted:146 147LGTM - How a Patch Is Accepted148------------------------------149 150A patch is approved to be committed when a reviewer accepts it, and this is151almost always associated with a message containing the text "LGTM" (which152stands for Looks Good To Me).153 154Only approval from a single reviewer is required, unless the pull request155has required reviewers. In which case, you must have approval from all of those156reviewers.157 158When providing an unqualified LGTM (approval to commit), it is the159responsibility of the reviewer to have reviewed all of the prior discussion and160feedback from all reviewers ensuring that all feedback has been addressed and161that all other reviewers will almost surely be satisfied with the patch being162approved. If unsure, the reviewer should provide a qualified approval, (e.g.,163"LGTM, but please wait for @someone, @someone_else"). You may also do this if164you are fairly certain that a particular community member will wish to review,165even if that person hasn't done so yet.166 167If additional feedback is provided after acceptance (by the same reviewer or168another), the author should use their best judgement in deciding whether that169feedback can be incorporated into the change without comment (say a typo) or170requires further review discussion. More substantial comments (e.g., about the171design) will usually require further discussion. If unsure, ask the reviewer.172 173Note that, if a reviewer has requested a particular community member to review,174and after a week that community member has yet to respond, feel free to ping175the patch (which literally means submitting a comment on the patch with the176word, "Ping."), or alternatively, ask the original reviewer for further177suggestions.178 179If it is likely that others will want to review a recently-posted patch,180especially if there might be objections, but no one else has done so yet, it is181also polite to provide a qualified approval (e.g., "LGTM, but please wait for a182couple of days in case others wish to review"). If approval is received very183quickly, a patch author may also elect to wait before committing (and this is184certainly considered polite for non-trivial patches). Especially given the185global nature of our community, this waiting time should be at least 24 hours.186Please also be mindful of weekends and major holidays.187 188Our goal is to ensure community consensus around design decisions and189significant implementation choices, and one responsibility of a reviewer, when190providing an overall approval for a patch, is to be reasonably sure that such191consensus exists. If you're not familiar enough with the community to know,192then you shouldn't be providing final approval to commit. A reviewer providing193final approval should have commit access to the LLVM project.194 195Every patch should be reviewed by at least one technical expert in the areas of196the project affected by the change.197 198Splitting Requests and Conditional Acceptance199---------------------------------------------200 201Reviewers may request certain aspects of a patch to be broken out into separate202patches for independent review. Reviewers may also accept a patch203conditioned on the author providing a follow-up patch addressing some204particular issue or concern (although no committed patch should leave the205project in a broken state). Moreover, reviewers can accept a patch conditioned on206the author applying some set of minor updates prior to committing, and when207applicable, it is polite for reviewers to do so.208 209Don't Unintentionally Block a Review210------------------------------------211 212If you review a patch, but don't intend for the review process to block on your213approval, please state that explicitly. Out of courtesy, we generally wait on214committing a patch until all reviewers are satisfied, and if you don't intend215to look at the patch again in a timely fashion, please communicate that fact in216the review.217 218Who Can/Should Review Code?219===========================220 221Non-Experts Should Review Code222------------------------------223 224You do not need to be an expert in some area of the code base to review patches;225it's fine to ask questions about what some piece of code is doing. If it's not226clear to you what is going on, you're unlikely to be the only one. Please227remember that it is not in the long-term best interest of the community to have228components that are only understood well by a small number of people. Extra229comments and/or test cases can often help (and asking for comments in the test230cases is fine as well).231 232Moreover, authors are encouraged to interpret questions as a reason to reexamine233the readability of the code in question. Structural changes, or further234comments, may be appropriate.235 236If you're new to the LLVM community, you might also find this presentation helpful:237.. _How to Contribute to LLVM, A 2019 LLVM Developers' Meeting Presentation: https://youtu.be/C5Y977rLqpw238 239A good way for new contributors to increase their knowledge of the code base is240to review code. It is perfectly acceptable to review code and explicitly241defer to others for approval decisions.242 243Experts Should Review Code244--------------------------245 246If you are an expert in an area of the compiler affected by a proposed patch,247then you are highly encouraged to review the code. If you are a relevant248maintainer, and no other experts are reviewing a patch, you must either help249arrange for an expert to review the patch or review it yourself.250 251Code Reviews, Speed, and Reciprocity252------------------------------------253 254Sometimes code reviews will take longer than you might hope, especially for255larger features. Common ways to speed up review times for your patches are:256 257* Review other people's patches. If you help out, everybody will be more258 willing to do the same for you; goodwill is our currency.259* Ping the patch. If it is urgent, provide reasons why it is important to you to260 get this patch landed and ping it every couple of days. If it is261 not urgent, the common courtesy ping rate is one week. Remember that you're262 asking for valuable time from other professional developers.263* Ask for help on Discord. Developers on Discord will be able to either help264 you directly, or tell you who might be a good reviewer.265* Split your patch into multiple smaller patches that build on each other. The266 smaller your patch is, the higher the probability that somebody will take a quick267 look at it. When doing this, it is helpful to add "[N/M]" (for 1 <= N <= M) to268 the title of each patch in the series, so it is clear that there is an order269 and what that order is.270 271Developers should participate in code reviews as both reviewers and272authors. If someone is kind enough to review your code, you should return the273favor for someone else. Note that anyone is welcome to review and give feedback274on a patch, but approval of patches should be consistent with the policy above.275 276Upstreaming Changes to LLVM277===========================278 279When upstreaming your own changes from a downstream project to LLVM, simply280follow the process outlined above.281 282When upstreaming changes originally written by someone else: 283 284* Ensure that there are no obstacles to upstreaming the code. In some cases,285 this simply means checking with the original author(s) to ensure they are286 aware of and approve the upstreaming. In other cases, licensing287 considerations may be more complex.288* Properly attribute the original changes, e.g., by creating a commit with289 multiple authors (`GitHub guide <https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors>`_).290* Invite the original author(s) to review the changes, and also include291 additional reviewers. Specifically, an LGTM from a (co-)author should not be292 taken as approval to land a change.293