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

[Mark de Wever via Phabricator via llvm-commits]

Mordante added a comment.

Thanks for writing this document! I noticed the document contains a lot of long sentences. These may be harder to understand for non-native speakers. I pointed a few out, but for some I don't have better suggestions.



================
Comment at: llvm/docs/CodeReview.rst:28
+Code can be reviewed either before it is committed or after. We expect
+significant changes to be reviewed before being committed, but smaller changes
+(or changes where the developer owns the component) that meet
----------------
Maybe change `committed, but smaller` to `committed. Smaller` in order to improve the readability?


================
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.
----------------
Maybe change `changes, and these` to `changes. These` in order to improve the readability?


================
Comment at: llvm/docs/CodeReview.rst:49
+Please note: There is no bar for post-commit feedback that is higher than for
+pre-commit feedback. Don't delay unnecessarily in providing feedback, but if
+you see something after code has been committed about which you would have
----------------
Maybe change `feedback, but if` to `feedback. If` in order to improve the readability?


================
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
----------------
Maybe change `choices, and one` to `choices. One` in order to improve the readability?


================
Comment at: llvm/docs/CodeReview.rst:146
+Reviewers may request certain aspects of a patch to be broken out into separate
+patches for independent review, and also, reviewers may accept a patch
+conditioned on the author providing a follow-up patch addressing some
----------------
Maybe change `review, and also, reviewers` to `review. Reviewers` in order to improve the readability?


================
Comment at: llvm/docs/CodeReview.rst:184
+A good way for new contributors to increase their knowledge of the code base is
+to review code, and it is perfectly acceptable to review code and explicitly
+defer to others for approval decisions.
----------------
Maybe change `code, and it` to `code. It` in order to improve the readability?


================
Comment at: llvm/docs/CodeReview.rst:207
+  asking for valuable time from other professional developers.
+* Ask for help on IRC. Developers on IRC will be able to either help you
+  directly, or tell you who might be a good reviewer.
----------------
MaskRay wrote:
> mehdi_amini wrote:
> > "Ask for help on IRC or Discord."?
> From https://lists.llvm.org/pipermail/llvm-dev/2019-November/136880.html , people are concerned about Discord's user policy. Not mentioning it here (before we reach a consensus) should be fine.
Add a link to the IRC channel?


================
Comment at: llvm/docs/CodeReview.rst:215
+authors. If someone is kind enough to review your code, you should return the
+favor for someone else.  Note that anyone is welcome to review and give feedback
+on a patch, but patches should be approved only consistent with the policy above.
----------------
There are two space here `else.  Note`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71916