<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 12/3/19 8:26 AM, Finkel, Hal J.
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:413ee54a-b830-990d-40d9-e86d77ac71bb@anl.gov">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p><br>
</p>
<div class="moz-cite-prefix">On 12/3/19 9:45 AM, David Blaikie via
llvm-dev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAENS6EvfUqyvhvsWgVjKYUhz9DYUO+dRN=vfr0b_B5Ot+Od2fw@mail.gmail.com">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at
8:23 PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 12/2/19 10:05 AM, David Blaikie wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Mon, Dec 2,
2019 at 12:52 PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
target="_blank" moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<p>Mehdi, David,</p>
<p>I think you're both pointing out exceptions
rather than the general rule. I tried to
indicate their might be reasonable
exceptions (see the second sentence past
Mehdi's quote), but in general, particularly
for new contributors, I think it is
important we indicate something to this
effect. I've seen multiple new groups have
issues around this. In some cases, patches
were reverted in post review. In others, a
bunch of time was sunk in a direction which
turned turned out not to have wide
agreement. Cautioning folks to avoid that
is important.</p>
<p>Do you have any suggestions on wording
which keep the broad message, but make it
more clear that it isn't a hard and fast
rule?</p>
</div>
</blockquote>
<div><br>
My take on it is that even the broad message
isn't how I think of LLVM code review practices
- if the code owner/domain expert is at your
company, so be it. If not, that's fine too - I
see lots of reviews by people at the same
company that generally look pretty good & I
don't think their the exception. I'd personally
leave this part out - maybe some caveat about
not doing internal review & then summarily
approving externally? But that I think is more
the exception than the rule & perhaps not
worth including in the general practices
document. But if you've seen several instances
of this sort of issue that seem worth addressing
through this mechanism - yeah, I'd be OK with an
encouragement to avoid insular project areas
where possible (especially where there's strong
overlap) - seek external/long-term contributor
input especially on design documents and
early/foundational patches, and in general err
on the side of seeking more input from code
owners than not. If you ask too often for
trivial things, that's fine - they should
hopefully get to the point where they encourage
you to contribute directly/stop asking for their
review/approval. But especially when asking code
owners/frequent reviewers/contributors for
review, be extra sure to make the patches small
and clear, to have design discussion ahead of
time to avoid designing in a large unwieldy code
review, etc. <br>
<br>
</div>
</div>
</div>
</blockquote>
Hm, I see your point, but I'm not 100% in agreement.
I'm trying to find the hair to split, but I'm struggling
to find the right one. Let's drop this for the moment
from the initial patch, and I'll try to a rework on the
wording in a separate review. I don't want to risk
further derailing the overall effort right now. </div>
</blockquote>
<div><br>
Fair - certainly up for chatting about it further to try
to come to some common understanding/phrasing/etc.<br>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>I feel like the underlying point is that we want to ensure
community consensus around design decisions, and one
responsibility of a reviewer, when providing an overall approval
for a patch, is to be reasonable sure that such consensus
exists. Maybe that means making sure that people from multiple
organizations likely to care about a particular issue have had a
chance to review, and/or have been specifically pinged, and
maybe not. One way of thinking about it may be that if you're
not familiar enough with the community to know, then you
shouldn't be providing final approval to commit.</p>
</blockquote>
I like this framing. It gets at the core point while avoiding the
concerns raised.<br>
<blockquote type="cite"
cite="mid:413ee54a-b830-990d-40d9-e86d77ac71bb@anl.gov">
<p>My preference is to start with some high-level guideline at
this level, and then we can develop more-specific guidelines as
needed. I don't really want to develop specific
organization-based rules, unless we really need to, in part
because the decomposition of legal entities is often arbitrary
in this context.</p>
<p> -Hal<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:CAENS6EvfUqyvhvsWgVjKYUhz9DYUO+dRN=vfr0b_B5Ot+Od2fw@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>- Dave<br>
</div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div>
<p>Philip<br>
</p>
<div>On 12/2/19 7:55 AM, David Blaikie wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Yeah, +1 that people from the
same organization are sometimes the only
ones working on a certain feature/area.
(certainly I'd expect some discussion
about the feature in general to be
discussed outside that group if it's in
any way contentious - but some stuff's
clear enough (I think I implemented
debug_types years ago, likely with only
Eric's approval, both of us being at
Google (probably many DWARF features were
added/done this way, to be honest - maybe
some could've done witha bit of broader
discussion, but I don't think either of us
were "rubber stamping" the other's work
(if anything I'm harder on my "friends" to
be honest... :/ )))</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed,
Nov 27, 2019 at 10:27 PM Mehdi AMINI via
llvm-dev <<a
href="mailto:llvm-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div dir="ltr">
<div><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On
Sat, Nov 16, 2019 at 5:56 PM
Mehdi AMINI <<a
href="mailto:joker.eph@gmail.com"
target="_blank"
moz-do-not-send="true">joker.eph@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div dir="ltr">+1 in general,
and Philip has good
suggestions as well!<br>
<div><br>
</div>
<div>-- </div>
<div>Mehdi</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr"
class="gmail_attr">On Sat,
Nov 16, 2019 at 8:37 AM
Philip Reames via llvm-dev
<<a
href="mailto:llvm-dev@lists.llvm.org"
target="_blank"
moz-do-not-send="true">llvm-dev@lists.llvm.org</a>>
wrote:<br>
</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>
<br>
On 11/14/19 7:46 PM, Finkel,
Hal J. via llvm-dev wrote:<br>
> Hi, everyone,<br>
><br>
> I've been fielding an
increasing number of
questions about how our <br>
> code-review process in
LLVM works from people who
are new to our <br>
> community, and it's
been pointed out to me that
our documentation on <br>
> code reviews is both
out of date and not as
helpful as it could be to <br>
> new developers.<br>
><br>
> <a
href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">
http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a><br>
><br>
> I would like to compose
a patch to update this, but
before I do that, I <br>
> want to highlight some
of my thoughts to get
feedback. My intent is to <br>
> capture our community
best practices in writing so
that people new to <br>
> our community
understand our processes and
expectations. Here are some
<br>
> things that I would
like to capture:<br>
><br>
> 1. You do not need to
be an expert in some area of
the compiler to <br>
> review patches; it's
fine to ask questions about
what some piece of code <br>
> is doing. If it's not
clear to you what is going
on, you're unlikely to <br>
> be the only one. Extra
comments and/or test cases
can often help (and <br>
> asking for comments in
the test cases is fine as
well).<br>
Authors are encouraged to
interpret questions as
reasons to reexamine<br>
the readability of the code
in question. Structural
changes, or further<br>
comments may be appropriate.<br>
><br>
> 2. If you review a
patch, but don't intend for
the review process to <br>
> block on your approval,
please state that
explicitly. Out of courtesy,
<br>
> we generally wait on
committing a patch until all
reviewers are <br>
> satisfied, and if you
don't intend to look at the
patch again in a <br>
> timely fashion, please
communicate that fact in the
review.<br>
><br>
> 3. All comments by
reviewers should be
addressed by the patch
author. <br>
> It is generally
expected that suggested
changes will be incorporated
<br>
> into the next revision
of the patch unless the
author and/or other <br>
> reviewers can
articulate a good reason to
do otherwise (and then the <br>
> reviewers must agree).
If you suggest changes in a
code review, but <br>
> don't wish the
suggestion to be interpreted
this strongly, please state
<br>
> so explicitly.<br>
><br>
> 4. Reviewers may
request certain aspects of a
patch to be broken out <br>
> into separate patches
for independent review, and
also, reviewers may <br>
> accept a patch
conditioned on the author
providing a follow-up patch
<br>
> addressing some
particular issue or concern
(although no committed patch
<br>
> should leave the
project in a broken state).
Reviewers can also accept a
<br>
> patch conditioned on
the author applying some set
of minor updates prior <br>
> to committing, and when
applicable, it is polite for
reviewers to do so.<br>
><br>
> 5. Aim to limit the
number of iterations in the
review process. For <br>
> example, when
suggesting a change, if you
want the author to make a <br>
> similar set of changes
at other places in the code,
please explain the <br>
> requested set of
changes so that the author
can make all of the changes
<br>
> at once. If a patch
will require multiple steps
prior to approval (e.g., <br>
> splitting, refactoring,
posting data from specific
performance tests), <br>
> please explain as many
of these up front as
possible. This allows the <br>
> patch author to make
the most-efficient use of
his or her time.<br>
If the path forward is not
clear - because the patch is
too large to<br>
meaningful review, or
direction needs to be
settled - it is fine to<br>
suggest a clear next step
(e.g. landing a refactoring)
followed by a<br>
re-review. Please state
explicitly if the path
forward is unclear to<br>
prevent confusions on the
part of the author. <br>
><br>
> 6. Some changes are
too large for just a code
review. Changes that <br>
> should change the
Language Reference (e.g.,
adding new <br>
> target-independent
intrinsics), adding language
extensions in Clang, and <br>
> so on, require an RFC
on *-dev first. For changes
that promise <br>
> significant impact on
users and/or downstream code
bases, reviewers can <br>
> request an RFC (Request
for Comment) achieving
consensus before <br>
> proceeding with code
review. That having been
said, posting initial <br>
> patches can help with
discussions on an RFC.<br>
><br>
> Lastly, the current
text reads, "Code reviews
are conducted by email on <br>
> the relevant project’s
commit mailing list, or
alternatively on the <br>
> project’s development
list or bug tracker.", and
then only later <br>
> mentions Phabricator.
I'd like to move Phabricator
to be mentioned on <br>
> this line before the
other methods.<br>
><br>
> Please let me know what
you think.<br>
><br>
> Thanks again,<br>
><br>
> Hal<br>
<br>
A couple of additional
things:<br>
<br>
Only a single LGTM is
required. Reviewers are
expected to only LGTM<br>
patches they're confident in
their knowledge of.
Reviewers may review<br>
and provide suggestions, but
explicitly defer LGTM to
someone else. <br>
This is encouraged and a
good way for new
contributors to learn the
code. <br>
<br>
There is a cultural
expectation that at least
one reviewer is from a<br>
different organization than
the author of the patch. </blockquote>
</div>
</blockquote>
<div><br>
</div>
<div>Actually, while I'm OK with
the other suggestions, I didn't
pay attention to this one
originally.</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div>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).</div>
<div><br>
</div>
<div>-- </div>
<div>Mehdi</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
<div class="gmail_quote">
<blockquote
class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
If that's not<br>
possible, care should be
taken to ensure overall
direction has been<br>
widely accepted. <br>
<br>
Post commit review is
encouraged via either
phabricator or email. There<br>
is a strong expectation that
authors respond promptly to
post commit<br>
feedback and address it.
Failure to do so is cause
for the patch to be<br>
reverted. If substantial
problems are identified, it
is expected that<br>
the patch is reverted, fixed
offline, and then
recommitted (possibly<br>
after further review.)<br>
<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a
href="mailto:llvm-dev@lists.llvm.org"
target="_blank"
moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
<a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev"
rel="noreferrer"
target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
<a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</blockquote>
</body>
</html>