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

Wed Nov 27 19:26:51 PST 2019

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
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191127/c9503d01/attachment.html>