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

David Blaikie via llvm-dev llvm-dev at lists.llvm.org
Mon Nov 18 08:58:17 PST 2019

On Mon, Nov 18, 2019 at 7:53 AM Finkel, Hal J. via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> On 11/18/19 4:29 AM, James Henderson wrote:
> 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.
> Whilst I get what you're trying to say, I'm not particularly comfortable
> with this particular suggestion as it stands: I regularly am one of two or
> three active reviewers on something, often spread out over different
> timezones, and I might be happy with it, in which case I'd signal that with
> an LGTM, but others might not be ready for it to be committed. Sometimes
> I'll ask for the author to wait to commit until one or more of the other
> reviewers are happy too, but other times I forget to explicitly say this.
> Perhaps a couple of sentences could be added to the end of this paragraph
> to capture this approach:
> "If multiple reviewers have been actively reviewing the patch, it is
> generally courteous to allow all of them a chance to give their LGTM before
> committing, after one LGTM has been received. The reviewer who gives the
> original LGTM may suggest an appropriate amount of time to wait before
> committing in this case."
> What I want to avoid is me (UK timezone) making some suggestions on a
> patch proposed by someone e.g. in the US, then a reviewer from the US
> getting into an active discussion, proposing a counter-suggestion, which
> gets adopted and LGTMed by that reviewer, resulting in a commit before I've
> had a chance to follow up on my comments etc. Obviously I can make
> post-commit requests, but sometimes it feels like the bar for suggestions
> post-commit is higher, and therefore my comments might not reach that level
> etc.
> I agree with this. I was planning on proposing wording along the lines of
> the following, adding to the original suggestion:
> When providing an unqualified LGTM (approval to commit), it is the
> responsibility of the reviewer to have reviewed all of the discussion and
> feedback from all reviewers ensuring that all feedback has been addressed
> and that all other reviewers will almost surely be satisfied with the patch
> being approved. If unsure, the reviewer should provide a qualified
> approval, (e.g., "LGTM, but please wait for @someone, @someone_else"). You
> may also do this if you are fairly certain that a particular community
> member will wish to review, even if that person hasn't done so yet
> (although don't wait for more than one week if that person has not
> responded; if you think something is "must see" by a wider audience, it
> should have an RFC). If it is likely that others will want to review a
> recently-posted patch, especially if there might be objections, but no one
> else has done so yet, it is also polite to provide a qualified approval
> (e.g., "LGTM, but please wait for a couple days in case others wish to
> review").
Generally sounds good to me though " (although don't wait for more than one
week if that person has not responded; if you think something is "must see"
by a wider audience, it should have an RFC)" - makes me a bit twitchy. I
think it's pretty reasonable for someone to say "LGTM, but also wait
for @someone" without having a week deadline. I do that pretty often -
hoping to provide a first pass review but there being a design decision or
the like that I feel is out of my depth and needs sign off from a code
owner or similar, essentially.

>  -Hal
> James
> On Sat, 16 Nov 2019 at 16:37, 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.  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
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> 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/20191118/e3b6b7c4/attachment-0001.html>

More information about the llvm-dev mailing list