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

Andrzej WarzyƄski via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 10 04:31:47 PDT 2024


================
@@ -150,14 +150,18 @@ 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
+merged), these need to be addressed and all new changes have to be reviewed and
+approved by a reviewer.
----------------
banach-space wrote:

I've expanded this paragraph. @Jakub, hopefully that meets your criteria (which I mostly agree with).

> what do you mean to cover here?

What should happen when there's a "LGTM", but then another reviewer leaves new comments afterwards? I wanted to clarify that the original "LGTM" wouldn't apply to the new comments (unless, like Jakub points out, those would be  basic nits etc).

> For example, when a new reviewer comes and add a typo/nit comment, I'd expect that the author can fix that and merge based on the previous approval given how insignificant the comment (and subsequent fix) is. 

This is assuming that there won't be more substantial comments following the nits - is it a valid assumption? I guess the reviewer should make that clear, e.g. "Just some nits, no need to wait for my approval".

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


More information about the llvm-commits mailing list