[llvm-dev] [RFC] High-Level Code-Review Documentation Update

Philip Reames via llvm-dev llvm-dev at lists.llvm.org
Mon Dec 2 17:23:36 PST 2019

On 12/2/19 10:05 AM, David Blaikie wrote:
> On Mon, Dec 2, 2019 at 12:52 PM Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>     Mehdi, David,
>     I think you're both pointing out exceptions rather than the
>     general rule.  I tried to indicate their might be reasonable
>     exceptions (see the second sentence past Mehdi's quote), but in
>     general, particularly for new contributors, I think it is
>     important we indicate something to this effect.  I've seen
>     multiple new groups have issues around this.  In some cases,
>     patches were reverted in post review.  In others, a bunch of time
>     was sunk in a direction which turned turned out not to have wide
>     agreement.  Cautioning folks to avoid that is important.
>     Do you have any suggestions on wording which keep the broad
>     message, but make it more clear that it isn't a hard and fast rule?
> My take on it is that even the broad message isn't how I think of LLVM 
> code review practices - if the code owner/domain expert is at your 
> company, so be it. If not, that's fine too - I see lots of reviews by 
> people at the same company that generally look pretty good & I don't 
> think their the exception. I'd personally leave this part out - maybe 
> some caveat about not doing internal review & then summarily approving 
> externally? But that I think is more the exception than the rule & 
> perhaps not worth including in the general practices document. But if 
> you've seen several instances of this sort of issue that seem worth 
> addressing through this mechanism - yeah, I'd be OK with an 
> encouragement to avoid insular project areas where possible 
> (especially where there's strong overlap) - seek external/long-term 
> contributor input especially on design documents and 
> early/foundational patches, and in general err on the side of seeking 
> more input from code owners than not. If you ask too often for trivial 
> things, that's fine - they should hopefully get to the point where 
> they encourage you to contribute directly/stop asking for their 
> review/approval. But especially when asking code owners/frequent 
> reviewers/contributors for review, be extra sure to make the patches 
> small and clear, to have design discussion ahead of time to avoid 
> designing in a large unwieldy code review, etc.
Hm, I see your point, but I'm not 100% in agreement.  I'm trying to find 
the hair to split, but I'm struggling to find the right one. Let's drop 
this for the moment from the initial patch, and I'll try to a rework on 
the wording in a separate review.  I don't want to risk further 
derailing the overall effort right now.
> - Dave
>     Philip
>     On 12/2/19 7:55 AM, David Blaikie wrote:
>>     Yeah, +1 that people from the same organization are sometimes the
>>     only ones working on a certain feature/area. (certainly I'd
>>     expect some discussion about the feature in general to be
>>     discussed outside that group if it's in any way contentious - but
>>     some stuff's clear enough (I think I implemented debug_types
>>     years ago, likely with only Eric's approval, both of us being at
>>     Google (probably many DWARF features were added/done this way, to
>>     be honest - maybe some could've done witha  bit of broader
>>     discussion, but I don't think either of us were "rubber stamping"
>>     the other's work (if anything I'm harder on my "friends" to be
>>     honest... :/ )))
>>     On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev
>>     <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>         On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI
>>         <joker.eph at gmail.com <mailto:joker.eph at gmail.com>> wrote:
>>             +1 in general, and Philip has good suggestions as well!
>>             -- 
>>             Mehdi
>>             On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via
>>             llvm-dev <llvm-dev at lists.llvm.org
>>             <mailto:llvm-dev at lists.llvm.org>> wrote:
>>                 + 1 in general, a couple of suggestions
>>                 On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
>>                 > Hi, everyone,
>>                 >
>>                 > I've been fielding an increasing number of
>>                 questions about how our
>>                 > code-review process in LLVM works from people who
>>                 are new to our
>>                 > community, and it's been pointed out to me that our
>>                 documentation on
>>                 > code reviews is both out of date and not as helpful
>>                 as it could be to
>>                 > new developers.
>>                 >
>>                 > http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>>                 >
>>                 > I would like to compose a patch to update this, but
>>                 before I do that, I
>>                 > want to highlight some of my thoughts to get
>>                 feedback. My intent is to
>>                 > capture our community best practices in writing so
>>                 that people new to
>>                 > our community understand our processes and
>>                 expectations. Here are some
>>                 > things that I would like to capture:
>>                 >
>>                 >   1. 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 clear to you what is going
>>                 on, you're unlikely to
>>                 > be the only one. Extra comments and/or test cases
>>                 can often help (and
>>                 > asking for comments in the test cases is fine as well).
>>                 Authors are encouraged to interpret questions as
>>                 reasons to reexamine
>>                 the readability of the code in question.  Structural
>>                 changes, or further
>>                 comments may be appropriate.
>>                 >
>>                 >   2. If you review a patch, but don't intend for
>>                 the review process to
>>                 > block on your approval, please state that
>>                 explicitly. Out of courtesy,
>>                 > we generally wait on committing a patch until all
>>                 reviewers are
>>                 > satisfied, and if you don't intend to look at the
>>                 patch again in a
>>                 > timely fashion, please communicate that fact in the
>>                 review.
>>                 >
>>                 >   3. All comments by reviewers should be addressed
>>                 by the patch author.
>>                 > It is generally expected that suggested changes
>>                 will be incorporated
>>                 > into the next revision of the patch unless the
>>                 author and/or other
>>                 > reviewers can articulate a good reason to do
>>                 otherwise (and then the
>>                 > reviewers must agree). If you suggest changes in a
>>                 code review, but
>>                 > don't wish the suggestion to be interpreted this
>>                 strongly, please state
>>                 > so explicitly.
>>                 >
>>                 >   4. Reviewers may request certain aspects of a
>>                 patch to be broken out
>>                 > into separate patches for independent review, and
>>                 also, reviewers may
>>                 > accept a patch conditioned on the author providing
>>                 a follow-up patch
>>                 > addressing some particular issue or concern
>>                 (although no committed patch
>>                 > should leave the project in a broken state).
>>                 Reviewers can also accept a
>>                 > patch conditioned on the author applying some set
>>                 of minor updates prior
>>                 > to committing, and when applicable, it is polite
>>                 for reviewers to do so.
>>                 >
>>                 >   5. Aim to limit the number of iterations in the
>>                 review process. For
>>                 > example, when suggesting a change, if you want the
>>                 author to make a
>>                 > similar set of changes at other places in the code,
>>                 please explain the
>>                 > requested set of changes so that 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
>>                 possible. This allows the
>>                 > patch author to make the most-efficient use of his
>>                 or her time.
>>                 If the path forward is not clear - because the patch
>>                 is too large to
>>                 meaningful review, or direction needs to be settled -
>>                 it is fine to
>>                 suggest a clear next step (e.g. landing a
>>                 refactoring) followed by a
>>                 re-review.  Please state explicitly if the path
>>                 forward is unclear to
>>                 prevent confusions on the part of the author.
>>                 >
>>                 >   6. Some changes are too large for just a code
>>                 review. Changes that
>>                 > should change the Language Reference (e.g., adding new
>>                 > target-independent intrinsics), adding language
>>                 extensions in Clang, and
>>                 > so on, require an RFC on *-dev first. For changes
>>                 that promise
>>                 > significant impact on users and/or downstream code
>>                 bases, reviewers can
>>                 > request an RFC (Request for Comment) achieving
>>                 consensus before
>>                 > proceeding with code review. That having been said,
>>                 posting initial
>>                 > patches can help with discussions on an RFC.
>>                 >
>>                 > Lastly, the current text reads, "Code reviews are
>>                 conducted by email on
>>                 > the relevant project’s commit mailing list, or
>>                 alternatively on the
>>                 > project’s development list or bug tracker.", and
>>                 then only later
>>                 > mentions Phabricator. I'd like to move Phabricator
>>                 to be mentioned on
>>                 > this line before the other methods.
>>                 >
>>                 > Please let me know what you think.
>>                 >
>>                 > Thanks again,
>>                 >
>>                 > Hal
>>                 A couple of additional things:
>>                 Only a single LGTM is required. Reviewers are
>>                 expected to only LGTM
>>                 patches they're confident in their knowledge of. 
>>                 Reviewers may review
>>                 and provide suggestions, but explicitly defer LGTM to
>>                 someone else.
>>                 This is encouraged and a good way for new
>>                 contributors to learn the code.
>>                 There is a cultural expectation that at least one
>>                 reviewer is from a
>>                 different organization than the author of the patch. 
>>         Actually, while I'm OK with the other suggestions, I didn't
>>         pay attention to this one originally.
>>         I'm very concerned about this: this looks like an assumption
>>         of bad faith or malice in the review process, and I find this
>>         unhealthy if it were part of the "cultural expectation".
>>         Moreover there are many areas of the compiler where there
>>         aren't many people available to review changes.
>>         I personally never really paid attention to who is the
>>         author/reviewer of a patch from an organizational point of
>>         view, I haven't perceived this culture of looking into
>>         affiliation so far. I never got the impression that reviewer
>>         were more difficult with me than they would be with others.
>>         There have been many patches that I reviewed that originated
>>         from other people from the same company as mine (back when I
>>         was at Apple mostly). The notion of "organization" is blurry:
>>         frequently this involved people from different teams inside
>>         the same company,  are they part of "the same organization"?
>>         Some of these people I have never even ever met or never
>>         heard of them before reviewing a patch (sometimes I don't
>>         even realize since there is a Phabricator pseudo and not
>>         everyone is using their business email here).
>>         -- 
>>         Mehdi
>>                 If that's not
>>                 possible, care should be taken to ensure overall
>>                 direction has been
>>                 widely accepted.
>>                 Post commit review is encouraged via either
>>                 phabricator or email.  There
>>                 is a strong expectation that authors respond promptly
>>                 to post commit
>>                 feedback and address it.  Failure to do so is cause
>>                 for the patch to be
>>                 reverted.  If substantial problems are identified, it
>>                 is expected that
>>                 the patch is reverted, fixed offline, and then
>>                 recommitted (possibly
>>                 after further review.)
>>                 _______________________________________________
>>                 LLVM Developers mailing list
>>                 llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>>                 https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>         _______________________________________________
>>         LLVM Developers mailing list
>>         llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>>         https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/34481b6d/attachment-0001.html>

More information about the llvm-dev mailing list