Page MenuHomePhabricator

High-Level Code-Review Documentation Update
ClosedPublic

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
11

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

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

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
208

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

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

68
``*-dev``
208

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.

211

smaller your patch is

218

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
49

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
49

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

132

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
49

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
68

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

69

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
29

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

36

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

50

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

135

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

147

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

185

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

208

Add a link to the IRC channel?

216

There are two space here else. Note

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

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.

59–60

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.

100

e.g., -> e.g.

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

102

most-efficient -> most efficient

129

couple days -> couple of days

169

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

211

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
47

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.

200

s/Accepted/Common/

211

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
100

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
19

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

36

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

60

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

62

"Is" should be capitalized.

89

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

105

"Is" should be capitalized.

135

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
147

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

150

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

217

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?

hfinkel marked 45 inline comments as done.Feb 28 2020, 9:05 PM

I would like to thank everyone who provided feedback on this patch. I apologize for the slow turn-around time on this. I'll upload a revised version that, I believe, addresses all feedback.

llvm/docs/CodeReview.rst
100

That's correct. This is, unfortunately, a traditional style difference between different parts of the world.

208

It's on the front page along with the mailing lists. If we're going to have links to the channel, I'd rather have a page on it so we can centralize that information (instead of duplicating it in various places throughout the documentation).

llvm/docs/DeveloperPolicy.rst
163

Thanks. I added it back in the LGTM section.

hfinkel updated this revision to Diff 247404.Feb 28 2020, 9:10 PM
hfinkel marked 2 inline comments as done.

Updated per review feedback. Please let me know if you see anything else that should be addressed pre-commit.

llvm/docs/CodeReview.rst
218

I am still seeing the empty line.

231

Is this meant to imply that the first and the final patches in a series should use a different convention?

hfinkel marked 3 inline comments as done.Feb 29 2020, 1:03 PM
hfinkel added inline comments.
llvm/docs/CodeReview.rst
231

No. Thanks for catching that.

hfinkel updated this revision to Diff 247459.Feb 29 2020, 1:05 PM
hfinkel marked an inline comment as done.

Updated to remove blank line and fix that (1 < N < M) should be (1 <= N <= M).

LGTM with one additional comment. Sorry for missing it earlier.

llvm/docs/Lexicon.rst
238

I am not very familiar with the format of the file, but it seems the new entry occurs between an entry for "Root" and some way of associating reference labels to that entry.

llvm/docs/CodeReview.rst
218

Hmm, it seems we hit one of the situations where Phabricator causes confusion. The empty line that @MaskRay requested removal of (https://reviews.llvm.org/D71916?id=235378#inline-651131) is the one at the end of the file (which is still present now). I am not sure if the empty line which did get removed is necessary in this document format.

jhenderson added inline comments.Mar 2 2020, 1:06 AM
llvm/docs/CodeReview.rst
231

Maybe:
what the order is -> what that order is

llvm/docs/Lexicon.rst
238

Yes, those look like link anchors to me, and probably belong with the **Root** block below.

241

Not sure if we have code-style guidelines for docs, but the 80 character limit looks like it is being applied here still, so this needs reflowing.

FWIW, I spell it out as "Request for Comment" in my mind! Don't think it matters though.

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

hfinkel marked 5 inline comments as done.Mar 4 2020, 10:03 PM

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

Thanks! - I've made a minimal update to this section now. I think it would be best to handle more movement there between documents in a separate patch.

hfinkel updated this revision to Diff 248392.Mar 4 2020, 10:04 PM

Update formatting, etc., and add a reference in the new-contributor section of the dev policy in response to reviewer feedback.

@hfinkel thanks a lot for this! (especially as a newcomer).
As you mentioned in llvm-dev, it might be a good idea to remove some "New Contributors" info from DeveloperPolicy and reference CodeReview.
In this case, I think it's good to put here the info about who commits what and what happens with the new contributors.

Thanks! - I've made a minimal update to this section now. I think it would be best to handle more movement there between documents in a separate patch.

Thanks, great. I'll wait for this to be committed. :)

jhenderson accepted this revision.Mar 5 2020, 2:38 AM

LGTM. Thanks!

LGTM. Thanks!

LGTM as well.

This revision was automatically updated to reflect the committed changes.