Skip to content

Commit 41248b5

Browse files
authored
[docs] Update docs on code-review process (#111735)
Clarify expectations for handling new comments post-LGTM but pre-commit. This change aims to standardize expectations when new comments are added after a patch has received LGTM but before it has been committed. Currently, approaches to this vary, and this update seeks to clarify best practices.
1 parent 6d719d9 commit 41248b5

File tree

2 files changed

+13
-13
lines changed

2 files changed

+13
-13
lines changed

llvm/docs/CodeReview.rst

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,14 +156,20 @@ has required reviewers. In which case, you must have approval from all of those
156156
reviewers.
157157

158158
When providing an unqualified LGTM (approval to commit), it is the
159-
responsibility of the reviewer to have reviewed all of the discussion and
159+
responsibility of the reviewer to have reviewed all of the prior discussion and
160160
feedback from all reviewers ensuring that all feedback has been addressed and
161161
that all other reviewers will almost surely be satisfied with the patch being
162162
approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
163163
"LGTM, but please wait for @someone, @someone_else"). You may also do this if
164164
you are fairly certain that a particular community member will wish to review,
165165
even if that person hasn't done so yet.
166166

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.
172+
167173
Note that, if a reviewer has requested a particular community member to review,
168174
and after a week that community member has yet to respond, feel free to ping
169175
the patch (which literally means submitting a comment on the patch with the

llvm/docs/Contributing.rst

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -102,21 +102,15 @@ will not be able to select reviewers in such a way, in which case you can still
102102
get the attention of potential reviewers by CC'ing them in a comment -- just
103103
@name them.
104104

105-
A reviewer may request changes or ask questions during the review. If you are
106-
uncertain on how to provide test cases, documentation, etc., feel free to ask
107-
for guidance during the review. Please address the feedback and re-post an
108-
updated version of your patch. This cycle continues until all requests and comments
109-
have been addressed and a reviewer accepts the patch with a `Looks good to me` or `LGTM`.
110-
Once that is done the change can be committed. If you do not have commit
111-
access, please let people know during the review and someone should commit it
112-
on your behalf.
113-
114105
If you have received no comments on your patch for a week, you can request a
115106
review by 'ping'ing the GitHub PR with "Ping". The common courtesy 'ping' rate
116-
is once a week. Please remember that you are asking for valuable time from other
117-
professional developers.
107+
is once a week. Please remember that you are asking for valuable time from
108+
other professional developers. Finally, if you do not have commit access,
109+
please let people know during the review and someone should commit it on your
110+
behalf once it has been accepted.
118111

119-
For more information on LLVM's code-review process, please see :doc:`CodeReview`.
112+
For more information on LLVM's code-review process, please see
113+
:doc:`CodeReview`.
120114

121115
.. _commit_from_git:
122116

0 commit comments

Comments
 (0)