llvm dev - Dec 2019 - [RFC] High-Level Code-Review Documentation Update

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... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>> +1 in general, and Philip has good suggestions as well!
>>
>> --
>> Mehdi
>>
>>
>> On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <
>> 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
>>> >
>>> > 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.
>>
>>
> Actually, while I'm OK with the other suggestions, I didn't pay
attention
> to this one originally.
> 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.
>
> 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.
> 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).
>
> --
> Mehdi
>
>
>
>
>
>> 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
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/2a360f09/attachment-0001.html>

yes, I remember marveling at how dblaikie and echristo would have occasionally
boisterous debates on-list when they essentially (possibly literally) sat next
to each other.

When my team started participating upstream, we were advised to try to avoid
getting approvals from each other, and specifically seek out reviewers from
other orgs.  Partly this is a matter of perspective, in that what’s good for one
org and what’s good for the upstream project may be fairly different, and that
may be clearer to someone from a different org.  I also think if a patch goes
through an internal review step before being posted upstream (a process we
employ a fair amount in my team) then it’s best if someone else does the
upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s
necessary to codify this too strongly, it’s more of a how-to-pick-reviewers
topic than how-we-do-reviews.

It would be more helpful if the Code Reviews section stated a goal (or short
goal set) for the review.  People can have radically different ideas about the
purpose of a code review.  I attended a software-engineering conference talk
once where Google people stated flat out that the sole purpose of their internal
reviews is readability.  Readability is nice, it fosters maintainability which
is certainly a good thing; but it goes against my entire career’s notion that
reviews primarily seek out logic holes and defects.
--paulr


From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of David
Blaikie via llvm-dev
Sent: Monday, December 2, 2019 10:56 AM
To: Mehdi AMINI <joker.eph at gmail.com>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

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... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> wrote:


On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at
gmail.com<mailto:joker.eph at gmail.com>> wrote:
+1 in general, and Philip has good suggestions as well!

--
Mehdi


On Sat, Nov 16, 2019 at 8:37 AM 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
>
> 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.

Actually, while I'm OK with the other suggestions, I didn't pay
attention to this one originally.
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.

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

--
Mehdi




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
_______________________________________________
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/0cdc4904/attachment.html>

On Mon, Dec 2, 2019 at 8:36 AM Robinson, Paul <paul.robinson at sony.com>
wrote:
> yes, I remember marveling at how dblaikie and echristo would have
> occasionally boisterous debates on-list when they essentially (possibly
> literally) sat next to each other.
>
>
>
> When my team started participating upstream, we were advised to try to
> avoid getting approvals from each other, and specifically seek out
> reviewers from other orgs.  Partly this is a matter of perspective, in that
> what’s good for one org and what’s good for the upstream project may be
> fairly different, and that may be clearer to someone from a different org.
> I also think if a patch goes through an internal review step before being
> posted upstream (a process we employ a fair amount in my team) then it’s
> best if someone else does the upstream review, just to avoid the
> rubber-stamp effect.  I don’t know that it’s necessary to codify this too
> strongly, it’s more of a how-to-pick-reviewers topic than
how-we-do-reviews.
>
>
>
> It would be more helpful if the Code Reviews section stated a goal (or
> short goal set) for the review.  People can have radically different ideas
> about the purpose of a code review.  I attended a software-engineering
> conference talk once where Google people stated flat out that the sole
> purpose of their internal reviews is readability.
>
The message was maybe not convey clearly? Google's internal review require
two kinds of approval and one of them is purely for "readability", and
not
everyone can give this approval (you need to get enough of your own patches
through first).
In my experience reviews are fairly aligned with Google's public
documentation on the topic: https://google.github.io/eng-practices/review/
(which I find to be a pretty good document by the way).

-- 
Mehdi

> Readability is nice, it fosters maintainability which is certainly a good
> thing; but it goes against my entire career’s notion that reviews primarily
> seek out logic holes and defects.
>
--paulr
>
>
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of
*David
> Blaikie via llvm-dev
> *Sent:* Monday, December 2, 2019 10:56 AM
> *To:* Mehdi AMINI <joker.eph at gmail.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] [RFC] High-Level Code-Review Documentation
> Update
>
>
>
> 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... :/ )))
>
>
>
> On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
>
>
>
> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
> +1 in general, and Philip has good suggestions as well!
>
>
>
> --
>
> Mehdi
>
>
>
>
>
> On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <
> 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
> >
> > 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.
>
>
>
> Actually, while I'm OK with the other suggestions, I didn't pay
attention
> to this one originally.
>
> 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.
>
>
>
> 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.
>
> 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).
>
>
>
> --
>
> Mehdi
>
>
>
>
>
>
>
>
>
> 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
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/72969645/attachment.html>

On Mon, Dec 2, 2019 at 11:36 AM Robinson, Paul <paul.robinson at sony.com>
wrote:
> yes, I remember marveling at how dblaikie and echristo would have
> occasionally boisterous debates on-list when they essentially (possibly
> literally) sat next to each other.
>
>
>
> When my team started participating upstream, we were advised to try to
> avoid getting approvals from each other, and specifically seek out
> reviewers from other orgs.  Partly this is a matter of perspective, in that
> what’s good for one org and what’s good for the upstream project may be
> fairly different, and that may be clearer to someone from a different org.
> I also think if a patch goes through an internal review step before being
> posted upstream (a process we employ a fair amount in my team) then it’s
> best if someone else does the upstream review, just to avoid the
> rubber-stamp effect.  I don’t know that it’s necessary to codify this too
> strongly, it’s more of a how-to-pick-reviewers topic than
how-we-do-reviews.
>
Yeah, it helps to do as much of that upstream as possible - even if it's
between two people from the same org. It helps upstream understand what
sort of scrutiny has already been applied to the patch (& allows others to
pipe up if it might be going down a path they don't agree with/might have
suggestions about)

Also there's a certain amount of the code ownership issue - & I /think/
that's more what came up (in my mind) with some contributions in the past:
Two relative newcomers to the project approving each others patches. That
to me is usually addressed by the "don't approve somtehing you
wouldn't've
committed yourself without review" - except at the code owner level where
owners/frequent contributors might be seeking feedback from each other on
design decisions with no clear hierarchy between them. But I realize that
gets a bit fuzzy/awkward.

>
>
> It would be more helpful if the Code Reviews section stated a goal (or
> short goal set) for the review.  People can have radically different ideas
> about the purpose of a code review.  I attended a software-engineering
> conference talk once where Google people stated flat out that the sole
> purpose of their internal reviews is readability.  Readability is nice, it
> fosters maintainability which is certainly a good thing; but it goes
> against my entire career’s notion that reviews primarily seek out logic
> holes and defects.
>
> --paulr
>
>
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of
*David
> Blaikie via llvm-dev
> *Sent:* Monday, December 2, 2019 10:56 AM
> *To:* Mehdi AMINI <joker.eph at gmail.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] [RFC] High-Level Code-Review Documentation
> Update
>
>
>
> 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... :/ )))
>
>
>
> On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
>
>
>
> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
> +1 in general, and Philip has good suggestions as well!
>
>
>
> --
>
> Mehdi
>
>
>
>
>
> On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <
> 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
> >
> > 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.
>
>
>
> Actually, while I'm OK with the other suggestions, I didn't pay
attention
> to this one originally.
>
> 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.
>
>
>
> 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.
>
> 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).
>
>
>
> --
>
> Mehdi
>
>
>
>
>
>
>
>
>
> 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
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/ecb202e3/attachment.html>

Mehdi, David,

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.

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?

Philip

On 12/2/19 7:55 AM, David Blaikie wrote:
> 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... :/ )))
>
> On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>
>
>
>     On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com
>     <mailto:joker.eph at gmail.com>> wrote:
>
>         +1 in general, and Philip has good suggestions as well!
>
>         -- 
>         Mehdi
>
>
>         On Sat, Nov 16, 2019 at 8:37 AM 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
>             >
>             > 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. 
>
>
>     Actually, while I'm OK with the other suggestions, I didn't pay
>     attention to this one originally.
>     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.
>
>     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.
>     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).
>
>     -- 
>     Mehdi
>
>
>
>             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
>
>     _______________________________________________
>     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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/12418fd9/attachment.html>

On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <listmail at
philipreames.com>
wrote:
> Mehdi, David,
>
> 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.
>
> 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?
>
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.

- Dave

> Philip
> On 12/2/19 7:55 AM, David Blaikie wrote:
>
> 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... :/ )))
>
> On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>>
>>
>> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at
gmail.com> wrote:
>>
>>> +1 in general, and Philip has good suggestions as well!
>>>
>>> --
>>> Mehdi
>>>
>>>
>>> On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <
>>> 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
>>>> >
>>>> > 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.
>>>
>>>
>> Actually, while I'm OK with the other suggestions, I didn't pay
attention
>> to this one originally.
>> 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.
>>
>> 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.
>> 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).
>>
>> --
>> Mehdi
>>
>>
>>
>>
>>
>>> 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
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/22449c8e/attachment-0001.html>