[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.


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