Page MenuHomePhabricator

High-Level Code-Review Documentation Update
AcceptedPublic

Authored by hfinkel on Dec 26 2019, 2:43 PM.

Details

Summary

An update to the documentation on our community code-review process. Based on the RFC: High-Level Code-Review Documentation Update (http://lists.llvm.org/pipermail/llvm-dev/2019-November/136808.html).

In this patch, I've pulled out the documentation into a separate file, and broken it into a number of subsections. I'm certainly open to feedback on both structure and content.

Diff Detail

Event Timeline

hfinkel created this revision.Dec 26 2019, 2:43 PM

Thanks again for doing this! As a general suggestion for code owners, "experts in areas", and people interested, I'd mention Herald rules to get notified if a patch touches a file or contains a keyword.

lattner accepted this revision.Dec 26 2019, 8:07 PM

This looks really great to me, thank you for driving this! I'd recommend giving this a couple of days for others to comment (as the doc itself suggest :-), given the global nature of the change. Thanks!

llvm/docs/CodeReview.rst
10

I'd recommend adding a fourth bullet, along the lines of:

  • Help grow and develop new contributors, through mentorship by community leaders.
122

Recommend s/him or her/them/ to be inclusive of our nonbinary friends.

This revision is now accepted and ready to land.Dec 26 2019, 8:07 PM

LGTM (but wait for the others contributors to the email threads, especially during this holidays season).

llvm/docs/CodeReview.rst
207

"Ask for help on IRC or Discord."?

mehdi_amini accepted this revision.Dec 26 2019, 9:29 PM
jdoerfert accepted this revision.Dec 26 2019, 9:42 PM

Just to make my approval formal: LGTM.

MaskRay accepted this revision.Dec 26 2019, 10:44 PM
MaskRay added inline comments.
llvm/docs/CodeReview.rst
48

The bar for post-commit feedback is not higher than pre-commit feedback ?

67
``*-dev``
207

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.

210

smaller your patch is

217

Delete the trailing empty line.

hfinkel marked an inline comment as done.Dec 27 2019, 12:11 AM
hfinkel added inline comments.
llvm/docs/CodeReview.rst
48

Do you disagree? This is an important question. I've never felt that there is a higher bar, but I've certainly heard people say that they feel otherwise in practice. I don't think that's optimal - if someone has feedback, we should hear it, and they shouldn't feel barred by the fact that the commit has happened already.

Clearly there a practical timing issue: if I comment on a patch after the committer has stopped working on the project, I might not receive any reply, and I'll really just need to submit a patch myself. If there are additional distinctions that we wish to capture, I'm certainly happy to try to do so.

aaron.ballman accepted this revision.Dec 27 2019, 9:44 AM

LGTM!

llvm/docs/CodeReview.rst
48

I think I read what @MaskRay wrote as a suggestion to improve the wording while keeping the same intent. The current phrasing is a bit awkward and I think the proposed wording is a bit better.

(FWIW, I agree that the bars are the same for pre- and post-commit feedback.)

131

Should we also mention something about keeping weekends and holidays in mind?

MaskRay added inline comments.Dec 27 2019, 9:50 AM
llvm/docs/CodeReview.rst
48

Sorry if I was unclear. I totally agree with this ("the bars are the same for pre- and post-commit feedback."). Just suggested an alternative wording..

modocache accepted this revision.Dec 27 2019, 10:06 AM
modocache added a subscriber: modocache.

Thanks for this! This LGTM but I have two non-blocking suggestions.

llvm/docs/CodeReview.rst
67

New contributors may not be able to glean that *-dev refers to one of a number of mailing lists. May I suggest changing this sentence slightly, to: "...and so on, require a "request for comments" (RFC) email to one of the LLVM project's *-dev mailing lists first."

68

I really appreciate spelling out the "RFC" acronym here, since brand-new contributors may not be familiar with this term. If I could make one or two small suggestions:

  1. "RFC" first appears in the sentence before this one, in "require an RFC on *-dev first." Maybe the parenthetical "(Request for Comment)" should appear there instead of in this sentence?
  2. "RFC" doesn't appear in the LLVM lexicon. Maybe you could add it, either as part of this patch, or in a separate commit?

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.

llvm/docs/CodeReview.rst
28

Maybe change committed, but smaller to committed. Smaller in order to improve the readability?

35

Maybe change changes, and these to changes. These in order to improve the readability?

49

Maybe change feedback, but if to feedback. If in order to improve the readability?

134

Maybe change choices, and one to choices. One in order to improve the readability?

146

Maybe change review, and also, reviewers to review. Reviewers in order to improve the readability?

184

Maybe change code, and it to code. It in order to improve the readability?

207

Add a link to the IRC channel?

215

There are two space here else. Note

jhenderson added inline comments.Jan 2 2020, 5:59 AM
llvm/docs/CodeReview.rst
51–52

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.

58–59

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.

99

e.g., -> e.g.

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

101

most-efficient -> most efficient

128

couple days -> couple of days

168

compiler -> code base
or similar - the LLVM project is now much more than a compiler :)

210

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.

Some inline comments, but otherwise, LGTM. Thanks!

llvm/docs/CodeReview.rst
46

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.

199

s/Accepted/Common/

210

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.

hubert.reinterpretcast marked an inline comment as done.Jan 2 2020, 9:05 AM
hubert.reinterpretcast added inline comments.
llvm/docs/CodeReview.rst
99

The use with the comma is consistent with the use and description in the MLA Handbook, 6th ed. My understanding is that the comma is customary in North American usage.

llvm/docs/CodeReview.rst
18

"Be" should be capitalized (as it is a few lines below).

35

I find changes, including those requested to be more readable.

59

The "alternatively" seems to be redundant (and hinders readability IMO).

61

"Is" should be capitalized.

88

"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.

104

"Is" should be capitalized.

134

The resulting second sentence (with "such consensus") refers to the first. The breaking of the flow by splitting the sentence reduces readability for me.

llvm/docs/CodeReview.rst
146

If breaking the sentence, then please retain "also": review. Reviewers may also.

149

For variety, I would suggest using something other than "also".

216

I believe this should be the adverb, "consistently", applying to "approved" as a verb. Alternatively: , but approval of patches should be consistent with the policy above.

llvm/docs/DeveloperPolicy.rst
163

Did the "write access"/"commit access" requirement survive?