[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