<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">One thing I wish our code review policy expressed was the intent of code reviews.<div class=""><br class=""></div><div class="">Personally, I believe the intent of code reviews is to improve the quality of code, to avoid the introduction of defects that another contributor could identify, and generally to benefit from the experience of other contributors.</div><div class=""><br class=""></div><div class="">One behavior that runs contrary to that intent (which has occasionally occurred in LLVM), is having your friend LGTM your patches as a rubber stamp. I'm not sure the right way to address that, but a clear statement of intent for code reviews could be a good first step.</div><div class=""><br class=""></div><div class="">-Chris<br class=""><div class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Nov 18, 2019, at 9:54 AM, Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class=""><p class="">All of the variations suggested seem reasonable to me. I minorly
prefer James' wording, but any of them are better than nothing. <br class="">
</p><p class="">Philip<br class="">
</p>
<div class="moz-cite-prefix">On 11/18/19 7:53 AM, Finkel, Hal J.
wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:1006cb80-7911-ad09-6b09-2956f79fa0a4@anl.gov" class="">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" class=""><p class=""><br class="">
</p>
<div class="moz-cite-prefix">On 11/18/19 4:29 AM, James Henderson
wrote:<br class="">
</div>
<blockquote type="cite" cite="mid:CABqSp3mRSiYFUrs-O=XzhQBtY5_3Hs+iB0cwFC7X0h=AzjCKLQ@mail.gmail.com" class="">
<div dir="ltr" class="">
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
Only a single LGTM is required. Reviewers are expected to
only LGTM<br class="">
patches they're confident in their knowledge of. Reviewers
may review<br class="">
and provide suggestions, but explicitly defer LGTM to
someone else. <br class="">
This is encouraged and a good way for new contributors to
learn the code. </blockquote>
<div class="">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:<br class="">
</div>
<div class=""><br class="">
</div>
<div class="">"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."</div>
<div class=""><br class="">
</div>
<div class="">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.</div>
</div>
</blockquote><p class=""><br class="">
</p><p class="">I agree with this. I was planning on proposing wording along
the lines of the following, adding to the original suggestion:</p><p class="">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").<br class="">
</p><p class="">Thoughts?</p><p class=""> -Hal<br class="">
</p><p class=""><br class="">
</p>
<blockquote type="cite" cite="mid:CABqSp3mRSiYFUrs-O=XzhQBtY5_3Hs+iB0cwFC7X0h=AzjCKLQ@mail.gmail.com" class="">
<div dir="ltr" class="">
<div class=""><br class="">
</div>
<div class="">James<br class="">
</div>
</div>
<br class="">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Sat, 16 Nov 2019 at
16:37, Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" moz-do-not-send="true" class="">llvm-dev@lists.llvm.org</a>>
wrote:<br class="">
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ 1 in general, a couple of suggestions<br class="">
<br class="">
On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:<br class="">
> Hi, everyone,<br class="">
><br class="">
> I've been fielding an increasing number of questions
about how our <br class="">
> code-review process in LLVM works from people who are
new to our <br class="">
> community, and it's been pointed out to me that our
documentation on <br class="">
> code reviews is both out of date and not as helpful as
it could be to <br class="">
> new developers.<br class="">
><br class="">
> <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" rel="noreferrer" target="_blank" moz-do-not-send="true" class="">
http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a><br class="">
><br class="">
> I would like to compose a patch to update this, but
before I do that, I <br class="">
> want to highlight some of my thoughts to get feedback.
My intent is to <br class="">
> capture our community best practices in writing so that
people new to <br class="">
> our community understand our processes and
expectations. Here are some <br class="">
> things that I would like to capture:<br class="">
><br class="">
> 1. You do not need to be an expert in some area of
the compiler to <br class="">
> review patches; it's fine to ask questions about what
some piece of code <br class="">
> is doing. If it's not clear to you what is going on,
you're unlikely to <br class="">
> be the only one. Extra comments and/or test cases can
often help (and <br class="">
> asking for comments in the test cases is fine as well).<br class="">
Authors are encouraged to interpret questions as reasons to
reexamine<br class="">
the readability of the code in question. Structural
changes, or further<br class="">
comments may be appropriate.<br class="">
><br class="">
> 2. If you review a patch, but don't intend for the
review process to <br class="">
> block on your approval, please state that explicitly.
Out of courtesy, <br class="">
> we generally wait on committing a patch until all
reviewers are <br class="">
> satisfied, and if you don't intend to look at the patch
again in a <br class="">
> timely fashion, please communicate that fact in the
review.<br class="">
><br class="">
> 3. All comments by reviewers should be addressed by
the patch author. <br class="">
> It is generally expected that suggested changes will be
incorporated <br class="">
> into the next revision of the patch unless the author
and/or other <br class="">
> reviewers can articulate a good reason to do otherwise
(and then the <br class="">
> reviewers must agree). If you suggest changes in a code
review, but <br class="">
> don't wish the suggestion to be interpreted this
strongly, please state <br class="">
> so explicitly.<br class="">
><br class="">
> 4. Reviewers may request certain aspects of a patch
to be broken out <br class="">
> into separate patches for independent review, and also,
reviewers may <br class="">
> accept a patch conditioned on the author providing a
follow-up patch <br class="">
> addressing some particular issue or concern (although
no committed patch <br class="">
> should leave the project in a broken state). Reviewers
can also accept a <br class="">
> patch conditioned on the author applying some set of
minor updates prior <br class="">
> to committing, and when applicable, it is polite for
reviewers to do so.<br class="">
><br class="">
> 5. Aim to limit the number of iterations in the
review process. For <br class="">
> example, when suggesting a change, if you want the
author to make a <br class="">
> similar set of changes at other places in the code,
please explain the <br class="">
> requested set of changes so that the author can make
all of the changes <br class="">
> at once. If a patch will require multiple steps prior
to approval (e.g., <br class="">
> splitting, refactoring, posting data from specific
performance tests), <br class="">
> please explain as many of these up front as possible.
This allows the <br class="">
> patch author to make the most-efficient use of his or
her time.<br class="">
If the path forward is not clear - because the patch is too
large to<br class="">
meaningful review, or direction needs to be settled - it is
fine to<br class="">
suggest a clear next step (e.g. landing a refactoring)
followed by a<br class="">
re-review. Please state explicitly if the path forward is
unclear to<br class="">
prevent confusions on the part of the author. <br class="">
><br class="">
> 6. Some changes are too large for just a code review.
Changes that <br class="">
> should change the Language Reference (e.g., adding new
<br class="">
> target-independent intrinsics), adding language
extensions in Clang, and <br class="">
> so on, require an RFC on *-dev first. For changes that
promise <br class="">
> significant impact on users and/or downstream code
bases, reviewers can <br class="">
> request an RFC (Request for Comment) achieving
consensus before <br class="">
> proceeding with code review. That having been said,
posting initial <br class="">
> patches can help with discussions on an RFC.<br class="">
><br class="">
> Lastly, the current text reads, "Code reviews are
conducted by email on <br class="">
> the relevant project’s commit mailing list, or
alternatively on the <br class="">
> project’s development list or bug tracker.", and then
only later <br class="">
> mentions Phabricator. I'd like to move Phabricator to
be mentioned on <br class="">
> this line before the other methods.<br class="">
><br class="">
> Please let me know what you think.<br class="">
><br class="">
> Thanks again,<br class="">
><br class="">
> Hal<br class="">
<br class="">
A couple of additional things:<br class="">
<br class="">
Only a single LGTM is required. Reviewers are expected to
only LGTM<br class="">
patches they're confident in their knowledge of. Reviewers
may review<br class="">
and provide suggestions, but explicitly defer LGTM to
someone else. <br class="">
This is encouraged and a good way for new contributors to
learn the code. <br class="">
<br class="">
There is a cultural expectation that at least one reviewer
is from a<br class="">
different organization than the author of the patch. If
that's not<br class="">
possible, care should be taken to ensure overall direction
has been<br class="">
widely accepted. <br class="">
<br class="">
Post commit review is encouraged via either phabricator or
email. There<br class="">
is a strong expectation that authors respond promptly to
post commit<br class="">
feedback and address it. Failure to do so is cause for the
patch to be<br class="">
reverted. If substantial problems are identified, it is
expected that<br class="">
the patch is reverted, fixed offline, and then recommitted
(possibly<br class="">
after further review.)<br class="">
<br class="">
<br class="">
_______________________________________________<br class="">
LLVM Developers mailing list<br class="">
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" moz-do-not-send="true" class="">llvm-dev@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" moz-do-not-send="true" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br class="">
</blockquote>
</div>
</blockquote>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</blockquote>
</div>
_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<br class=""></div></blockquote></div><br class=""></div></div></body></html>