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

Mon Dec 2 18:21:36 PST 2019

On Mon, Dec 2, 2019 at 10:05 AM David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <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'm aligned with David on this, and this sentence summarizes fairly well my
view as well.
I wasn't trying to point at the exception: I just never saw "belonging to
the same organization" as a very important discriminator in our review
process. I pick the best reviewer regardless, and I review patches in my
area similarly.

Best,

-- 
Mehdi

> 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.
>
> - 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> wrote:
>>
>>>
>>>
>>> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <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> 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
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>
>>>> _______________________________________________
>>> LLVM Developers mailing list
>>> 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/1a2fa97e/attachment.html>