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

Mon Dec 2 10:05:20 PST 2019

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 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/22449c8e/attachment-0001.html>