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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CodeReview.rst:51-52
+you see something after code has been committed about which you would have
+commented pre-commit (had you noticed it earlier), please feel free to provide
+that feedback at any time.
+
----------------
I don't know whether this is needed, but it might be worth noting that if a substantial period of time has passed since the original commit was made, it may be better to create a new patch to address the issues than comment on the original commit.


================
Comment at: llvm/docs/CodeReview.rst:58-59
+Code reviews are conducted on our web-based code-review tool (see
+:doc:`Phabricator`), by email on the relevant project's commit mailing list, or
+alternatively on the project's development list or bug tracker.
+
----------------
What is meant by "development list" here? Also, it's not clear if these three methods are considered equally acceptable at this point, or whether one should be preferred over the other for any given situation.


================
Comment at: llvm/docs/CodeReview.rst:99
+the author can make all of the changes at once. If a patch will require
+multiple steps prior to approval (e.g., splitting, refactoring, posting data
+from specific performance tests), please explain as many of these up front as
----------------
e.g., -> e.g.

Same goes for all other instances of this pattern. Alternatively, consider using "for example,"


================
Comment at: llvm/docs/CodeReview.rst:101
+from specific performance tests), please explain as many of these up front as
+possible. This allows the patch author and reviewers to make the most-efficient
+use of their time.
----------------
most-efficient -> most efficient


================
Comment at: llvm/docs/CodeReview.rst:128
+also polite to provide a qualified approval (e.g., "LGTM, but please wait for a
+couple days in case others wish to review"). If approval is received very
+quickly, a patch author may also elect to wait before committing (and this is
----------------
couple days -> couple of days


================
Comment at: llvm/docs/CodeReview.rst:168
+
+You do not need to be an expert in some area of the compiler to review patches;
+it's fine to ask questions about what some piece of code is doing. If it's not
----------------
compiler -> code base
or similar - the LLVM project is now much more than a compiler :)


================
Comment at: llvm/docs/CodeReview.rst:210
+* Split your patch into multiple smaller patches that build on each other. The
+  smaller your patch, the higher the probability that somebody will take a quick
+  look at it.
----------------
MaskRay wrote:
> smaller your patch //is//
Actually, I believe this phrasing is perfectly reasonable English. That being said, if the lack of "is" is confusing for a non-native speaker, there's nothing wrong with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71916