[llvm] [docs] Update docs on code-review process (PR #111735)
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 10 07:56:26 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:
This advise talks about action the reviewer should take, that seems backwards here. Unless I'm misreading, the problematic situation here is a prior LGTM followed by changes by a *different* reviewer and the author landing the patch without addressing those changes.
I'd suggest something along the lines of the following:
Only approval from a single reviewer is required; however the patch author is always responsible for making sure that all feedback on a review has been properly addressed or responded to. 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. Similarly, the patch author is responsible for incorporating post commit feedback on the change.
I'd put this paragraph just above the one talking about unqualified LGTM
https://github.com/llvm/llvm-project/pull/111735
More information about the llvm-commits
mailing list