[llvm-dev] [RFC] High-Level Code-Review Documentation Update
Philip Reames via llvm-dev
llvm-dev at lists.llvm.org
Tue Dec 3 10:30:40 PST 2019
On 12/3/19 8:26 AM, Finkel, Hal J. wrote:
>
>
> On 12/3/19 9:45 AM, David Blaikie via llvm-dev wrote:
>>
>>
>> On Mon, Dec 2, 2019 at 8:23 PM Philip Reames
>> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>>
>>
>> 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.
>>
>>
>> Fair - certainly up for chatting about it further to try to come to
>> some common understanding/phrasing/etc.
>
>
> I feel like the underlying point is that we want to ensure community
> consensus around design decisions, and one responsibility of a
> reviewer, when providing an overall approval for a patch, is to be
> reasonable sure that such consensus exists. Maybe that means making
> sure that people from multiple organizations likely to care about a
> particular issue have had a chance to review, and/or have been
> specifically pinged, and maybe not. One way of thinking about it may
> be that if you're not familiar enough with the community to know, then
> you shouldn't be providing final approval to commit.
>
I like this framing. It gets at the core point while avoiding the
concerns raised.
>
> My preference is to start with some high-level guideline at this
> level, and then we can develop more-specific guidelines as needed. I
> don't really want to develop specific organization-based rules, unless
> we really need to, in part because the decomposition of legal entities
> is often arbitrary in this context.
>
> -Hal
>
>
>>> - 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
>>>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191203/83fdc29a/attachment-0001.html>
More information about the llvm-dev
mailing list