[llvm] [docs] Update docs on code-review process (PR #111735)

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 10 09:04:39 PDT 2024


================
@@ -150,14 +150,21 @@ almost always associated with a message containing the text "LGTM" (which
 stands for Looks Good To Me). Only approval from a single reviewer is required.
 
 When providing an unqualified LGTM (approval to commit), it is the
-responsibility of the reviewer to have reviewed all of the discussion and
+responsibility of the reviewer to have reviewed all of the prior discussion and
 feedback from all reviewers ensuring that all feedback has been addressed and
 that all other reviewers will almost surely be satisfied with the patch being
 approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
 "LGTM, but please wait for @someone, @someone_else"). You may also do this if
 you are fairly certain that a particular community member will wish to review,
 even if that person hasn't done so yet.
 
+If new comments are posted after the patch has been approved (but not yet
----------------
preames wrote:

> > Unless I'm misreading, the problematic situation here is a prior LGTM followed by comments by a different reviewer and the author landing the patch without addressing those comments.
> 
> Yes
> 
> > (edit: for typo fix)
> 
> No, for more substantial changes. For "typos", I added an exception.

(You're responding to a note I made about editting my text to fix a typo...)
 
> > If additional feedback is provided after acceptance (by the same reviewer or another), the author should use their best judgement in deciding whether that feedback can be incorporated into the change without comment (say a typo) or requires further review discussion.
> 
> To me this sounds that any comments "post LGTM" are "second category" (i.e. "for the author to decide whether to address them or not" vs "need to be addressed"). I'm suggesting that we should treat such comments as any other comments prior to LGTM (i.e., "need to be addressed"). I feel that that otherwise we are discouraging people from reading/commenting on patches post LGTM.

If you have suggested wording, feel free to suggest.  I don't read this the way you do, particular following the prior sentence that you trimmed.  



https://github.com/llvm/llvm-project/pull/111735


More information about the llvm-commits mailing list