[llvm] [docs] Update docs on code-review process (PR #111735)
Andrzej Warzyński via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 26 05:50:40 PDT 2024
https://github.com/banach-space updated https://github.com/llvm/llvm-project/pull/111735
>From a9f5488396d094c8551eac68809312a05dfea799 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/7] [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 3714f26e2a0529d68b18b2957f64c67a86b8bba6 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/7] 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.
>From d545001b60994401a093152b6ce5c8d9175f8e9c Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Wed, 9 Oct 2024 21:02:58 +0100
Subject: [PATCH 3/7] fixup! fixup! [docs] Update docs on code-review process
Remove paragraph from Contributing.rst
---
llvm/docs/Contributing.rst | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/llvm/docs/Contributing.rst b/llvm/docs/Contributing.rst
index 17477d1c044d76..c7f3e0a5e7155f 100644
--- a/llvm/docs/Contributing.rst
+++ b/llvm/docs/Contributing.rst
@@ -102,21 +102,9 @@ will not be able to select reviewers in such a way, in which case you can still
get the attention of potential reviewers by CC'ing them in a comment -- just
@name them.
-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
-access, please let people know during the review and someone should commit it
-on your behalf.
-
-If you have received no comments on your patch for a week, you can request a
-review by 'ping'ing the GitHub PR with "Ping". The common courtesy 'ping' rate
-is once a week. Please remember that you are asking for valuable time from other
-professional developers.
-
-For more information on LLVM's code-review process, please see :doc:`CodeReview`.
+If you do not have commit access, please let people know during the review and
+someone should commit it on your behalf once it has been accepted. For more
+information on LLVM's code-review process, please see :doc:`CodeReview`.
.. _commit_from_git:
>From 3df98cd7ddfca4ad63a4c3cbec7f771657b7559a Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Thu, 10 Oct 2024 12:23:57 +0100
Subject: [PATCH 4/7] fixup! fixup! fixup! [docs] Update docs on code-review
process
Expand the new paragraph
---
llvm/docs/CodeReview.rst | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 24f6ce86782964..6b7dbb77f29a94 100644
--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -159,8 +159,11 @@ 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.
+merged), these need to be addressed following similar process as outlined
+above. Specifically, a reviewer should confirm that all feedback has been
+addressed before a patch is merged, including the newly posted comments.
+Exceptions apply - e.g. there's no need to confirm that a comment requesting a
+typo to be fixed has been addressed (this should be evident from the code).
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
>From 3d646d15114138211274d7c9f12de7591e147be3 Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Fri, 11 Oct 2024 16:01:05 +0100
Subject: [PATCH 5/7] fixup! fixup! fixup! fixup! [docs] Update docs on
code-review process
Incorporate PR feedback
---
llvm/docs/CodeReview.rst | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 6b7dbb77f29a94..05554775dd2c80 100644
--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -158,12 +158,11 @@ 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 following similar process as outlined
-above. Specifically, a reviewer should confirm that all feedback has been
-addressed before a patch is merged, including the newly posted comments.
-Exceptions apply - e.g. there's no need to confirm that a comment requesting a
-typo to be fixed has been addressed (this should be evident from the code).
+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. More substantial comments (e.g. about the
+design) will usually require further discussion. If unsure, ask the 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
>From 97de0ec932698c5344f9e95ba4dc46d1d31c13b0 Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Mon, 14 Oct 2024 15:03:44 +0100
Subject: [PATCH 6/7] fixup! fixup! fixup! fixup! fixup! [docs] Update docs on
code-review process
Restore a note on weekly pings.
---
llvm/docs/Contributing.rst | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/llvm/docs/Contributing.rst b/llvm/docs/Contributing.rst
index c7f3e0a5e7155f..d503f1bcca1fcd 100644
--- a/llvm/docs/Contributing.rst
+++ b/llvm/docs/Contributing.rst
@@ -102,9 +102,15 @@ will not be able to select reviewers in such a way, in which case you can still
get the attention of potential reviewers by CC'ing them in a comment -- just
@name them.
-If you do not have commit access, please let people know during the review and
-someone should commit it on your behalf once it has been accepted. For more
-information on LLVM's code-review process, please see :doc:`CodeReview`.
+If you have received no comments on your patch for a week, you can request a
+review by 'ping'ing the GitHub PR with "Ping". The common courtesy 'ping' rate
+is once a week. Please remember that you are asking for valuable time from
+other professional developers. Finally, if you do not have commit access,
+please let people know during the review and someone should commit it on your
+behalf once it has been accepted.
+
+For more information on LLVM's code-review process, please see
+:doc:`CodeReview`.
.. _commit_from_git:
>From d8c6019adf8cb4530272809b6d549003d1fedd81 Mon Sep 17 00:00:00 2001
From: Andrzej Warzynski <andrzej.warzynski at arm.com>
Date: Sat, 26 Oct 2024 13:50:15 +0100
Subject: [PATCH 7/7] fixup! fixup! fixup! fixup! fixup! fixup! [docs] Update
docs on code-review process
Add missing coma
---
llvm/docs/CodeReview.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/docs/CodeReview.rst b/llvm/docs/CodeReview.rst
index 05554775dd2c80..4ae77c996526c9 100644
--- a/llvm/docs/CodeReview.rst
+++ b/llvm/docs/CodeReview.rst
@@ -161,7 +161,7 @@ even if that person hasn't done so yet.
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. More substantial comments (e.g. about the
+requires further review discussion. More substantial comments (e.g., about the
design) will usually require further discussion. If unsure, ask the reviewer.
Note that, if a reviewer has requested a particular community member to review,
More information about the llvm-commits
mailing list