[llvm-dev] [RFC] High-Level Code-Review Documentation Update
Philip Reames via llvm-dev
llvm-dev at lists.llvm.org
Mon Nov 18 09:54:50 PST 2019
All of the variations suggested seem reasonable to me. I minorly prefer
James' wording, but any of them are better than nothing.
Philip
On 11/18/19 7:53 AM, Finkel, Hal J. 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").
>
> Thoughts?
>
> -Hal
>
>
>>
>> James
>>
>> On Sat, 16 Nov 2019 at 16:37, 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
>> <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 <mailto: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/20191118/876b94d2/attachment.html>
More information about the llvm-dev
mailing list