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