<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">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">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><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">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">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">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">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">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">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">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote></div></div>