[PATCH] D71916: High-Level Code-Review Documentation Update

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/docs/CodeReview.rst:18
+
+What Code Should be Reviewed?
+-----------------------------
----------------
"Be" should be capitalized (as it is a few lines below).


================
Comment at: llvm/docs/CodeReview.rst:35
+Please note that the developer responsible for a code change is also
+responsible for making all necessary review-related changes, and these include
+changes requested during any post-commit review.
----------------
Mordante wrote:
> Maybe change `changes, and these` to `changes. These` in order to improve the readability?
I find `changes, including those requested` to be more readable.


================
Comment at: llvm/docs/CodeReview.rst:59
+:doc:`Phabricator`), by email on the relevant project's commit mailing list, or
+alternatively on the project's development list or bug tracker.
+
----------------
The "alternatively" seems to be redundant (and hinders readability IMO).


================
Comment at: llvm/docs/CodeReview.rst:61
+
+When is an RFC Required?
+------------------------
----------------
"Is" should be capitalized.


================
Comment at: llvm/docs/CodeReview.rst:88
+does not address all outstanding feedback, the author should explicitly state
+that in the patch description. If you suggest changes in a code review, but
+don't wish the suggestion to be interpreted this strongly, please state so
----------------
"Patch description" is ambiguous between the description of the "Differential revision" and a specific "diff". Depending on whether the author intends to address the feedback in a later commit or just a later update for the current review, one or the other is appropriate.


================
Comment at: llvm/docs/CodeReview.rst:104
+
+LGTM - How a Patch is Accepted
+------------------------------
----------------
"Is" should be capitalized.


================
Comment at: llvm/docs/CodeReview.rst:134
+Our goal is to ensure community consensus around design decisions and
+significant implementation choices, and one responsibility of a reviewer, when
+providing an overall approval for a patch, is to be reasonably sure that such
----------------
Mordante wrote:
> Maybe change `choices, and one` to `choices. One` in order to improve the readability?
The resulting second sentence (with "such consensus") refers to the first. The breaking of the flow by splitting the sentence reduces readability for me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71916/new/

https://reviews.llvm.org/D71916