Page MenuHomePhabricator

High-Level Code-Review Documentation Update

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



An update to the documentation on our community code-review process. Based on the RFC: High-Level Code-Review Documentation Update (

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!


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

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

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


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

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


From , people are concerned about Discord's user policy. Not mentioning it here (before we reach a consensus) should be fine.


smaller your patch is


Delete the trailing empty line.

hfinkel marked an inline comment as done.Dec 27 2019, 12:11 AM
hfinkel added inline comments.

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



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


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

MaskRay added inline comments.Dec 27 2019, 9:50 AM

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.


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


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.


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


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


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


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


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


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


Add a link to the IRC channel?


There are two space here else. Note

jhenderson added inline comments.Jan 2 2020, 5:59 AM

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.


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.


e.g., -> e.g.

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


most-efficient -> most efficient


couple days -> couple of days


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


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!


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.




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.

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.


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


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


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


"Is" should be capitalized.


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


"Is" should be capitalized.


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


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


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


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.


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