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

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Some inline comments, but otherwise, LGTM. Thanks!



================
Comment at: llvm/docs/CodeReview.rst:46
+expected that the patch is reverted, fixed offline, and then recommitted
+(possibly after further review).
+
----------------
We want to avoid the pattern where a developer uses the buildbots as a trial and error strategy. This is particularly troublesome for slower builders (test-suite, multi-stage, slower/saturated builders).

I would add a recommendation that any patch that has failed in a particular area, and had a member of the community identify and revert, to be reviewed by such member (or the code owner) before it is recommitted.

In particular, I would like this recommendation to be even stronger when the patch doesn't change between iterations.

This means the author is relying on flaky explanations for recommitting and may not be considering that their patch could be the one creating the flakyness. I have seen this happening which had repercussions months later and it was a lot harder to fix the overall problem.


================
Comment at: llvm/docs/CodeReview.rst:199
+Sometimes code reviews will take longer than you might hope, especially for
+larger features. Accepted ways to speed up review times for your patches are:
+
----------------
s/Accepted/Common/


================
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.
----------------
jhenderson wrote:
> 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.
I'd also recommend adding "[N/M]" (for 1 < N < M) on the series, so it is clear that there is an order and what the order is.

Phab is not "phab" in making the order explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71916