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

Andrzej WarzyƄski via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 9 12:42:35 PDT 2024


https://github.com/banach-space updated https://github.com/llvm/llvm-project/pull/111735

>From a7db2b78bfb00b63cad59f7f93115dfd720c9cc5 Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Wed, 9 Oct 2024 19:26:27 +0100
Subject: [PATCH 1/2] [docs] Update docs on code-review process

Clarify what to do in cases where multiple reviewers commented on a
patch, but only one reviewer explicitly accepted.

This is mostly to standardize what the expectations are as folks tend to
approach this differently.
---
 llvm/docs/Contributing.rst | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/llvm/docs/Contributing.rst b/llvm/docs/Contributing.rst
index 17477d1c044d76..d3a86c0ad98229 100644
--- a/llvm/docs/Contributing.rst
+++ b/llvm/docs/Contributing.rst
@@ -105,9 +105,16 @@ get the attention of potential reviewers by CC'ing them in a comment -- just
 A reviewer may request changes or ask questions during the review. If you are
 uncertain on how to provide test cases, documentation, etc., feel free to ask
 for guidance during the review. Please address the feedback and re-post an
-updated version of your patch. This cycle continues until all requests and comments
-have been addressed and a reviewer accepts the patch with a `Looks good to me` or `LGTM`.
-Once that is done the change can be committed. If you do not have commit
+updated version of your patch. This cycle continues until all requests and
+comments have been addressed and the reviewer accepts the patch with a `Looks
+good to me` or `LGTM`. With multiple active reviewers (i.e. reviewers who left
+comments), it is good practice to seek `LGTM` from all of them. Alternatively, 
+when e.g. some reviewers go radio silent and you are blocked:
+* any reviewer can confirm that all comments from other reviewers have been
+  addressed and the change is ready to land, or
+* if you decide not to wait for explicit `LGTM` from other reviewers, please
+  leave a short justification.
+Once a change has been approved, it can be committed. If you do not have commit
 access, please let people know during the review and someone should commit it
 on your behalf.
 

>From 6d41647dbbe3f9c7608a6285166f1858b177d3e3 Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Wed, 9 Oct 2024 20:42:21 +0100
Subject: [PATCH 2/2] fixup! [docs] Update docs on code-review process

Revert changes in llvm/docs/Contributing.rst, update llvm/docs/CodeReview.rst instead.
---
 llvm/docs/CodeReview.rst   |  6 +++++-
 llvm/docs/Contributing.rst | 13 +++----------
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 56798ae4faf0c4..24f6ce86782964 100644
--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -150,7 +150,7 @@ 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.,
@@ -158,6 +158,10 @@ approved. If unsure, the reviewer should provide a qualified approval, (e.g.,
 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.
+
 Note that, if a reviewer has requested a particular community member to review,
 and after a week that community member has yet to respond, feel free to ping
 the patch (which literally means submitting a comment on the patch with the
diff --git a/llvm/docs/Contributing.rst b/llvm/docs/Contributing.rst
index d3a86c0ad98229..17477d1c044d76 100644
--- a/llvm/docs/Contributing.rst
+++ b/llvm/docs/Contributing.rst
@@ -105,16 +105,9 @@ get the attention of potential reviewers by CC'ing them in a comment -- just
 A reviewer may request changes or ask questions during the review. If you are
 uncertain on how to provide test cases, documentation, etc., feel free to ask
 for guidance during the review. Please address the feedback and re-post an
-updated version of your patch. This cycle continues until all requests and
-comments have been addressed and the reviewer accepts the patch with a `Looks
-good to me` or `LGTM`. With multiple active reviewers (i.e. reviewers who left
-comments), it is good practice to seek `LGTM` from all of them. Alternatively, 
-when e.g. some reviewers go radio silent and you are blocked:
-* any reviewer can confirm that all comments from other reviewers have been
-  addressed and the change is ready to land, or
-* if you decide not to wait for explicit `LGTM` from other reviewers, please
-  leave a short justification.
-Once a change has been approved, it can be committed. If you do not have commit
+updated version of your patch. This cycle continues until all requests and comments
+have been addressed and a reviewer accepts the patch with a `Looks good to me` or `LGTM`.
+Once that is done the change can be committed. If you do not have commit
 access, please let people know during the review and someone should commit it
 on your behalf.
 



More information about the llvm-commits mailing list