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

Andrzej Warzyński via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 14 07:37:14 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
----------------
banach-space wrote:

> You have seemingly managed to complete miss the spirit of my original suggestion

That's not intentional. Would you be able to share a diff so that I better understand what you had in mind? 

> If the document implies that the author is not responsible for incorporating feedback

I don’t believe the document implies that. I think it’s fairly clear that:

1. The author is responsible for addressing/incorporating the feedback, which is already covered under "Acknowledge All Reviewer Feedback" (so perhaps we can leave that as is).
2. The reviewer, when posting an unqualified LGTM, is responsible for ensuring that all feedback has been addressed (i.e., “I've reviewed all comments and confirm they’ve been addressed").

The existing wording covers those points well. The part that is unclear IMO is what happens **after LGTM** and **before commit** when new comments are posted. Hopefully, the additional paragraph I proposed clarifies that.

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


More information about the llvm-commits mailing list