Formalize "revert for more design review" policy.

Project Infrastructure LLVM Dev List Archives
1

Recently there’s been some friction over reversions (I can remember two cases in recent memory). In both issues the general feel I got is that as a community we should honor “revert for more design review” requests unconditionally.

What do you guys think of adding something like this to DeveloperPolicy.rst as an item at the end of the numbered list in http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
If a developer thinks that a patch would benefit from some more review
and promptly communicates this, the patch should be reverted (preferably
by the original author, unless they are unresponsive).
Developers often disagree, and erring on the side of the developer
asking for more review prevents any lingering disagreement over code in
the tree.

“promptly” is there mostly to avoid suggesting a “necro-revert”; once the code has been in tree for long enough at some point it would be more appropriate to open a bug report or start a fresh discussion.

“unresponsive” add some nebulousness, but I think it’s an important exception to call out for the “preferably by the original author”.

– Sean Silva

revertpolicy.patch (1.15 KB)

2

+Hal, who somehow slipped through the cracks.

3

From: "Sean Silva" <chisophugis@gmail.com>
To: "llvm-dev" <llvm-dev@lists.llvm.org>
Cc: "Chris Lattner" <clattner@apple.com>, "Rafael Ávila de Espíndola" <rafael.espindola@gmail.com>, "Michael Spencer"
<bigcheesegs@gmail.com>, "Chandler Carruth" <chandlerc@gmail.com>, "David Li" <davidxl@google.com>, "David Blaikie"
<dblaikie@gmail.com>, "Daniel Berlin" <dberlin@dberlin.org>, "Easwaran Raman" <eraman@google.com>, "Hal Finkel"
<hfinkel@anl.gov>
Sent: Tuesday, March 8, 2016 11:03:09 PM
Subject: Re: Formalize "revert for more design review" policy.

+Hal, who somehow slipped through the cracks.

Thanks :slight_smile:

Recently there's been some friction over reversions (I can remember
two cases in recent memory). In both issues the general feel I got
is that as a community we should honor "revert for more design
review" requests unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
If a developer thinks that a patch would benefit from some more
review
and promptly communicates this, the patch should be reverted
(preferably
by the original author, unless they are unresponsive).
Developers often disagree, and erring on the side of the developer
asking for more review prevents any lingering disagreement over code
in
the tree.

"promptly" is there mostly to avoid suggesting a "necro-revert"; once
the code has been in tree for long enough at some point it would be
more appropriate to open a bug report or start a fresh discussion.

"unresponsive" add some nebulousness, but I think it's an important
exception to call out for the "preferably by the original author".

I think this generally sounds right, and matches what we currently expect in practice. To this we might add that it is then the responsibility of the developer requesting the revert to ensure that the review happens promptly.

Do we have a formal policy for other kinds of reverts? We also generally revert if someone claims to have bisected a miscompile to a particular commit, and this is true even if a test case is not immediately available (although we do need some kind of reproducer before too long).

-Hal

4

> From: "Sean Silva" <chisophugis@gmail.com>
> To: "llvm-dev" <llvm-dev@lists.llvm.org>
> Cc: "Chris Lattner" <clattner@apple.com>, "Rafael Ávila de Espíndola" <
rafael.espindola@gmail.com>, "Michael Spencer"
> <bigcheesegs@gmail.com>, "Chandler Carruth" <chandlerc@gmail.com>,
"David Li" <davidxl@google.com>, "David Blaikie"
> <dblaikie@gmail.com>, "Daniel Berlin" <dberlin@dberlin.org>, "Easwaran
Raman" <eraman@google.com>, "Hal Finkel"
> <hfinkel@anl.gov>
> Sent: Tuesday, March 8, 2016 11:03:09 PM
> Subject: Re: Formalize "revert for more design review" policy.
>
>
> +Hal, who somehow slipped through the cracks.
>

Thanks :slight_smile:

>
>
>
>
> Recently there's been some friction over reversions (I can remember
> two cases in recent memory). In both issues the general feel I got
> is that as a community we should honor "revert for more design
> review" requests unconditionally.
>
>
> What do you guys think of adding something like this to
> DeveloperPolicy.rst as an item at the end of the numbered list in
> http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?
>
>
>
> #. Sometimes patches get committed that need more discussion.
> If a developer thinks that a patch would benefit from some more
> review
> and promptly communicates this, the patch should be reverted
> (preferably
> by the original author, unless they are unresponsive).
> Developers often disagree, and erring on the side of the developer
> asking for more review prevents any lingering disagreement over code
> in
> the tree.
>
>
> "promptly" is there mostly to avoid suggesting a "necro-revert"; once
> the code has been in tree for long enough at some point it would be
> more appropriate to open a bug report or start a fresh discussion.
>
>
> "unresponsive" add some nebulousness, but I think it's an important
> exception to call out for the "preferably by the original author".
>

I think this generally sounds right, and matches what we currently expect
in practice. To this we might add that it is then the responsibility of the
developer requesting the revert to ensure that the review happens promptly.

I think this is slightly problematic - it's not uncommon that we have new
contributors send something for review, then without waiting very long (by
our community standards - which is admittedly quite a while, but seen this
in under a week, etc) and then commit. This usually results in a knee-jerk
"please revert".

Committing prematurely shouldn't be a way to cause a priority increase for
a patch.

But, yeah, that doesn't necessarily address the sort of situations that
instigated this thread either. Probably reasonable to expect someone to say
/something/ in a review after a certain point, if they want to review it.

(one piece of the review puzzle that may also not be formalized: generally
I treat review approval as being "I would feel comfortable committing this
without review" (ie: two people who wouldn't independently commit a patch
don't become able to commit it by consensus - but maybe that's not a bad
thing to allow, just not the way I've generally treated code review))

Do we have a formal policy for other kinds of reverts? We also generally
revert if someone claims to have bisected a miscompile to a particular
commit, and this is true even if a test case is not immediately available
(although we do need some kind of reproducer before too long).

Yeah, this gets into interesting territory as well - if the problem is bad
enough, we do generally accept reverting a patch without a reproducer being
currently available (but, as you say, expecting it to come along shortly
thereafter).

5

Recently there's been some friction over reversions (I can remember two
cases in recent memory). In both issues the general feel I got is that as a
community we should honor "revert for more design review" requests
unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
   If a developer thinks that a patch would benefit from some more review
   and promptly communicates this, the patch should be reverted (preferably
   by the original author, unless they are unresponsive).
   Developers often disagree, and erring on the side of the developer
   asking for more review prevents any lingering disagreement over code in
   the tree.

This is an interesting proposal. In practice, what I have seen is that
developers usually send out RFC (design of some kinds) to llvm-dev long
before the actual implementation, and the patch is submitted after RFC did
not get any objections. Would the late request like this come as a surprise
to the developer?

thanks,

David

6

One data point: “FP Environment and Rounding mode handling” (see http://lists.llvm.org/pipermail/llvm-dev/2016-February/094869.html ).
There was a discussion on LLVM Dev, then a bunch of patches went through code review for months, and finally Chandler chimed in very late and we totally blocked the feature on a design ground.
It can be painful, it is case-by-case tradeoff to evaluate of course, but this is also what makes LLVM robust and relatively clean IMO.

FWIW I like Sean proposal, and I agree with David about the “priority increase” glitch.

7

From: "David Blaikie" <dblaikie@gmail.com>
To: "Hal Finkel" <hfinkel@anl.gov>
Cc: "Sean Silva" <chisophugis@gmail.com>, "Chris Lattner"
<clattner@apple.com>, "Rafael Ávila de Espíndola"
<rafael.espindola@gmail.com>, "Michael Spencer"
<bigcheesegs@gmail.com>, "Chandler Carruth" <chandlerc@gmail.com>,
"David Li" <davidxl@google.com>, "Daniel Berlin"
<dberlin@dberlin.org>, "Easwaran Raman" <eraman@google.com>,
"llvm-dev" <llvm-dev@lists.llvm.org>
Sent: Tuesday, March 8, 2016 11:22:11 PM
Subject: Re: Formalize "revert for more design review" policy.

> > From: "Sean Silva" < chisophugis@gmail.com >

> > To: "llvm-dev" < llvm-dev@lists.llvm.org >

> > Cc: "Chris Lattner" < clattner@apple.com >, "Rafael Ávila de
> > Espíndola" < rafael.espindola@gmail.com >, "Michael Spencer"

> > < bigcheesegs@gmail.com >, "Chandler Carruth" <
> > chandlerc@gmail.com
> > >, "David Li" < davidxl@google.com >, "David Blaikie"

> > < dblaikie@gmail.com >, "Daniel Berlin" < dberlin@dberlin.org >,
> > "Easwaran Raman" < eraman@google.com >, "Hal Finkel"

> > < hfinkel@anl.gov >

> > Sent: Tuesday, March 8, 2016 11:03:09 PM

> > Subject: Re: Formalize "revert for more design review" policy.

> >

> >

> > +Hal, who somehow slipped through the cracks.

> >

> Thanks :slight_smile:

> >

> >

> >

> >

> > Recently there's been some friction over reversions (I can
> > remember

> > two cases in recent memory). In both issues the general feel I
> > got

> > is that as a community we should honor "revert for more design

> > review" requests unconditionally.

> >

> >

> > What do you guys think of adding something like this to

> > DeveloperPolicy.rst as an item at the end of the numbered list in

> > http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

> >

> >

> >

> > #. Sometimes patches get committed that need more discussion.

> > If a developer thinks that a patch would benefit from some more

> > review

> > and promptly communicates this, the patch should be reverted

> > (preferably

> > by the original author, unless they are unresponsive).

> > Developers often disagree, and erring on the side of the
> > developer

> > asking for more review prevents any lingering disagreement over
> > code

> > in

> > the tree.

> >

> >

> > "promptly" is there mostly to avoid suggesting a "necro-revert";
> > once

> > the code has been in tree for long enough at some point it would
> > be

> > more appropriate to open a bug report or start a fresh
> > discussion.

> >

> >

> > "unresponsive" add some nebulousness, but I think it's an
> > important

> > exception to call out for the "preferably by the original
> > author".

> >

> I think this generally sounds right, and matches what we currently
> expect in practice. To this we might add that it is then the
> responsibility of the developer requesting the revert to ensure
> that
> the review happens promptly.

I think this is slightly problematic - it's not uncommon that we have
new contributors send something for review, then without waiting
very long (by our community standards - which is admittedly quite a
while, but seen this in under a week, etc) and then commit. This
usually results in a knee-jerk "please revert".

Committing prematurely shouldn't be a way to cause a priority
increase for a patch.

Completely agree. Although that does not fall into the "revert for more design review" category.

But, yeah, that doesn't necessarily address the sort of situations
that instigated this thread either. Probably reasonable to expect
someone to say /something/ in a review after a certain point, if
they want to review it.

(one piece of the review puzzle that may also not be formalized:
generally I treat review approval as being "I would feel comfortable
committing this without review" (ie: two people who wouldn't
independently commit a patch don't become able to commit it by
consensus - but maybe that's not a bad thing to allow, just not the
way I've generally treated code review))

Interesting point. I think we generally expect someone with expertise and experience in a particular area to approve substantial patches. For smaller fixups or enhancements, that might not be necessary. We normally rely on the honor system and the comfort level of the reviewer to allow for self selection on expertise and experience, and I'm unsure if/how we should try to formalize that.

Thanks again,
Hal

8

Recently there's been some friction over reversions (I can remember two
cases in recent memory). In both issues the general feel I got is that as a
community we should honor "revert for more design review" requests
unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
   If a developer thinks that a patch would benefit from some more review
   and promptly communicates this, the patch should be reverted
(preferably
   by the original author, unless they are unresponsive).
   Developers often disagree, and erring on the side of the developer
   asking for more review prevents any lingering disagreement over code in
   the tree.

This is an interesting proposal. In practice, what I have seen is that
developers usually send out RFC (design of some kinds) to llvm-dev long
before the actual implementation, and the patch is submitted after RFC did
not get any objections. Would the late request like this come as a surprise
to the developer?

One data point: "FP Environment and Rounding mode handling" (see
http://lists.llvm.org/pipermail/llvm-dev/2016-February/094869.html ).
There was a discussion on LLVM Dev, then a bunch of patches went through
code review for months, and finally Chandler chimed in very late and we
totally blocked the feature on a design ground.
It can be painful, it is case-by-case tradeoff to evaluate of course, but
this is also what makes LLVM robust and relatively clean IMO.

This is a very good example of well thought/concrete concerns that can
trigger great discussions. Holding off the feature for discussion in this
scenario is well justified and the absolute the right thing to do.

David

9

Recently there's been some friction over reversions (I can remember two
cases in recent memory). In both issues the general feel I got is that as a
community we should honor "revert for more design review" requests
unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
   If a developer thinks that a patch would benefit from some more review
   and promptly communicates this, the patch should be reverted
(preferably
   by the original author, unless they are unresponsive).
   Developers often disagree, and erring on the side of the developer
   asking for more review prevents any lingering disagreement over code in
   the tree.

This is an interesting proposal. In practice, what I have seen is that
developers usually send out RFC (design of some kinds) to llvm-dev long
before the actual implementation, and the patch is submitted after RFC did
not get any objections.

This is generally the case for large/significant changes. I think this
would have worked well for the recent PGO inliner patch; I think much of
the issues there were miscommunication that would have been resolved with
an RFC (e.g. that it was intentional to reimplement some small parts of the
new pass manager and will be removed once the legacy PassManager is gone).
An RFC also increases awareness of the patch which may attract reviewers or
accelerate review.

Even if the basic ideas have been discussed in the past, having a fresh RFC
on LLVMDev that evolves directly into the patch review on llvm-commits
seems most natural.

Would the late request like this come as a surprise to the developer?

In the case of a very large or important patch with extensive RFC
discussion, I think that the "promptly" condition would be very gray and we
would need to evaluate on a case-by-case basis. Generally I feel like there
is a very good explanation for why the developer making the revert request
was unable to participate in the original discussion in order for the
revert to make sense.

The "surprise" is more likely for smaller patches where one developer
thought it was nonproblematic and so thought that post-commit review would
be appropriate. If another developer sees what they perceive to be serious
issues I think it is reasonable to revert and initiate a pre-commit review.
I think the situation of r260488 would have fallen under this category.

-- Sean Silva

10

Recently there's been some friction over reversions (I can remember two
cases in recent memory). In both issues the general feel I got is that as a
community we should honor "revert for more design review" requests
unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
   If a developer thinks that a patch would benefit from some more review
   and promptly communicates this, the patch should be reverted
(preferably
   by the original author, unless they are unresponsive).
   Developers often disagree, and erring on the side of the developer
   asking for more review prevents any lingering disagreement over code
in
   the tree.

This is an interesting proposal. In practice, what I have seen is that
developers usually send out RFC (design of some kinds) to llvm-dev long
before the actual implementation, and the patch is submitted after RFC did
not get any objections.

This is generally the case for large/significant changes. I think this
would have worked well for the recent PGO inliner patch; I think much of
the issues there were miscommunication that would have been resolved with
an RFC (e.g. that it was intentional to reimplement some small parts of the
new pass manager and will be removed once the legacy PassManager is gone).
An RFC also increases awareness of the patch which may attract reviewers
or accelerate review.

Even if the basic ideas have been discussed in the past, having a fresh
RFC on LLVMDev that evolves directly into the patch review on llvm-commits
seems most natural.

Would the late request like this come as a surprise to the developer?

In the case of a very large or important patch with extensive RFC
discussion, I think that the "promptly" condition would be very gray and we
would need to evaluate on a case-by-case basis. Generally I feel like there
is a very good explanation for why the developer making the revert request
was unable to participate in the original discussion in order for the
revert to make sense.

For clarity:
s/Generally I feel like there is/Generally I feel like there should be/

-- Sean Silva

11

> From: "Sean Silva" <chisophugis@gmail.com>
> To: "llvm-dev" <llvm-dev@lists.llvm.org>
> Cc: "Chris Lattner" <clattner@apple.com>, "Rafael Ávila de Espíndola" <
rafael.espindola@gmail.com>, "Michael Spencer"
> <bigcheesegs@gmail.com>, "Chandler Carruth" <chandlerc@gmail.com>,
"David Li" <davidxl@google.com>, "David Blaikie"
> <dblaikie@gmail.com>, "Daniel Berlin" <dberlin@dberlin.org>, "Easwaran
Raman" <eraman@google.com>, "Hal Finkel"
> <hfinkel@anl.gov>
> Sent: Tuesday, March 8, 2016 11:03:09 PM
> Subject: Re: Formalize "revert for more design review" policy.
>
>
> +Hal, who somehow slipped through the cracks.
>

Thanks :slight_smile:

>
>
>
>
> Recently there's been some friction over reversions (I can remember
> two cases in recent memory). In both issues the general feel I got
> is that as a community we should honor "revert for more design
> review" requests unconditionally.
>
>
> What do you guys think of adding something like this to
> DeveloperPolicy.rst as an item at the end of the numbered list in
> http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?
>
>
>
> #. Sometimes patches get committed that need more discussion.
> If a developer thinks that a patch would benefit from some more
> review
> and promptly communicates this, the patch should be reverted
> (preferably
> by the original author, unless they are unresponsive).
> Developers often disagree, and erring on the side of the developer
> asking for more review prevents any lingering disagreement over code
> in
> the tree.
>
>
> "promptly" is there mostly to avoid suggesting a "necro-revert"; once
> the code has been in tree for long enough at some point it would be
> more appropriate to open a bug report or start a fresh discussion.
>
>
> "unresponsive" add some nebulousness, but I think it's an important
> exception to call out for the "preferably by the original author".
>

I think this generally sounds right, and matches what we currently expect
in practice. To this we might add that it is then the responsibility of the
developer requesting the revert to ensure that the review happens promptly.

I think this is slightly problematic - it's not uncommon that we have new
contributors send something for review, then without waiting very long (by
our community standards - which is admittedly quite a while, but seen this
in under a week, etc) and then commit. This usually results in a knee-jerk
"please revert".

Committing prematurely shouldn't be a way to cause a priority increase for
a patch.

I think we should assume good faith, which makes this a non-issue.

But, yeah, that doesn't necessarily address the sort of situations that
instigated this thread either. Probably reasonable to expect someone to say
/something/ in a review after a certain point, if they want to review it.

(one piece of the review puzzle that may also not be formalized: generally
I treat review approval as being "I would feel comfortable committing this
without review" (ie: two people who wouldn't independently commit a patch
don't become able to commit it by consensus - but maybe that's not a bad
thing to allow, just not the way I've generally treated code review))

Do we have a formal policy for other kinds of reverts? We also generally
revert if someone claims to have bisected a miscompile to a particular
commit, and this is true even if a test case is not immediately available
(although we do need some kind of reproducer before too long).

Yeah, this gets into interesting territory as well - if the problem is bad
enough, we do generally accept reverting a patch without a reproducer being
currently available (but, as you say, expecting it to come along shortly
thereafter).

I have thought a bit about this, and I think it is sufficiently nuanced to
probably be worth a separate discussion.

-- Sean Silva

12

Recently there's been some friction over reversions (I can remember two
cases in recent memory). In both issues the general feel I got is that as a
community we should honor "revert for more design review" requests
unconditionally.

What do you guys think of adding something like this to
DeveloperPolicy.rst as an item at the end of the numbered list in
http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?

#. Sometimes patches get committed that need more discussion.
   If a developer thinks that a patch would benefit from some more review
   and promptly communicates this, the patch should be reverted
(preferably
   by the original author, unless they are unresponsive).
   Developers often disagree, and erring on the side of the developer
   asking for more review prevents any lingering disagreement over code
in
   the tree.

This is an interesting proposal. In practice, what I have seen is that
developers usually send out RFC (design of some kinds) to llvm-dev long
before the actual implementation, and the patch is submitted after RFC did
not get any objections.

This is generally the case for large/significant changes. I think this
would have worked well for the recent PGO inliner patch; I think much of
the issues there were miscommunication that would have been resolved with
an RFC (e.g. that it was intentional to reimplement some small parts of the
new pass manager and will be removed once the legacy PassManager is gone).
An RFC also increases awareness of the patch which may attract reviewers
or accelerate review.

Even if the basic ideas have been discussed in the past, having a fresh
RFC on LLVMDev that evolves directly into the patch review on llvm-commits
seems most natural.

In hindsight, the refresh RFC should have been done, giving that various
discussions have been going on for more than a year ..

thanks,

David

13

Slightly OT, but I wanted to raise a related point for clarity. I’ve noticed a general trend to interpret RFCs without response as approval to move forward. This is not what it means; more often its that either a) the RFC is so off base no one has the time to explain why or b) no one is interested enough in the feature to respond. It’s the responsibility of the RFC author to get explicit agreement. This can be annoying and frustrating, but it is necessary. Most of the late design blockages I’ve seen over the last couple of months fall into this category. Philip

14

+1 to the general proposal. A couple of small word tweaking suggestions: 1) we should emphasize this is a “no fault” situation. i.e. it’s not that the author did anything wrong per se. 2) “thinks that a patch” → “thinks that a recently committed patch” + drop the promptly bit 3) “should be reverted” → “should be reverted promptly”. 4) Add a sentence: “Reverting a patch ensures that design discussions can happen without blocking other development; it’s entirely possible the patch will end up being reapplied essentially as is once concerns have been resolved.”

15

> From: "Sean Silva" <chisophugis@gmail.com>
> To: "llvm-dev" <llvm-dev@lists.llvm.org>
> Cc: "Chris Lattner" <clattner@apple.com>, "Rafael Ávila de Espíndola" <
rafael.espindola@gmail.com>, "Michael Spencer"
> <bigcheesegs@gmail.com>, "Chandler Carruth" <chandlerc@gmail.com>,
"David Li" <davidxl@google.com>, "David Blaikie"
> <dblaikie@gmail.com>, "Daniel Berlin" <dberlin@dberlin.org>, "Easwaran
Raman" <eraman@google.com>, "Hal Finkel"
> <hfinkel@anl.gov>
> Sent: Tuesday, March 8, 2016 11:03:09 PM
> Subject: Re: Formalize "revert for more design review" policy.
>
>
> +Hal, who somehow slipped through the cracks.
>

Thanks :slight_smile:

>
>
>
>
> Recently there's been some friction over reversions (I can remember
> two cases in recent memory). In both issues the general feel I got
> is that as a community we should honor "revert for more design
> review" requests unconditionally.
>
>
> What do you guys think of adding something like this to
> DeveloperPolicy.rst as an item at the end of the numbered list in
> http://llvm.org/docs/DeveloperPolicy.html#code-reviews ?
>
>
>
> #. Sometimes patches get committed that need more discussion.
> If a developer thinks that a patch would benefit from some more
> review
> and promptly communicates this, the patch should be reverted
> (preferably
> by the original author, unless they are unresponsive).
> Developers often disagree, and erring on the side of the developer
> asking for more review prevents any lingering disagreement over code
> in
> the tree.
>
>
> "promptly" is there mostly to avoid suggesting a "necro-revert"; once
> the code has been in tree for long enough at some point it would be
> more appropriate to open a bug report or start a fresh discussion.
>
>
> "unresponsive" add some nebulousness, but I think it's an important
> exception to call out for the "preferably by the original author".
>

I think this generally sounds right, and matches what we currently expect
in practice. To this we might add that it is then the responsibility of the
developer requesting the revert to ensure that the review happens promptly.

I think this is slightly problematic - it's not uncommon that we have new
contributors send something for review, then without waiting very long (by
our community standards - which is admittedly quite a while, but seen this
in under a week, etc) and then commit. This usually results in a knee-jerk
"please revert".

Committing prematurely shouldn't be a way to cause a priority increase for
a patch.

But, yeah, that doesn't necessarily address the sort of situations that
instigated this thread either. Probably reasonable to expect someone to say
/something/ in a review after a certain point, if they want to review it.

This is slightly OT to the subject of this thread, but +1 to "Probably
reasonable to expect someone to say /something/ in a review after a certain
point, if they want to review it.". Even a "I don't have the time to give
detailed review comments right now, but I have concerns about this patch"
puts the onus on the submitter to work with the reviewer to find out and
address the concerns.

16

I agree with the paragraph, and most of the comments thereafter. The
intention to clarify reverts as an evolutionary process instead of a
punishment is most welcome.

Without reverts we can't have post-commit reviews in any way,
especially with so many downstream projects depending on LLVM.

Code and bot owners may be more trigger happy than other members, but
that's because they care about the overall status of their part in the
compiler and/or their architectures, and that's an important point of
view that should not be taken lightly.

I believe our community works mostly well in regards to commits,
reviews and reverts, but it's good to encode at least the general idea
so that new members don't feel bad about having their patches
reverted.

cheers,
-renato

17

On that note, I'd suggest some sort of policy for responding to RFCs. From the RFC proponent's perspective, it is a lot better to get any kind of feedback within a predictable time frame than to get none at all. It doesn't have to be a detailed analysis, a general opinion would often be sufficient to see if the idea should be pursued further or not.

-Krzysztof

18

Hi,

It seems like there was a general agreement in this thread, and our current written guidelines say “In addition, if substantial problems are identified, it is expected that the patch is reverted and fixed offline”, which may not be explicit enough.

With the previous suggestions from Philip’s incorporated in Sean’s proposal, I amended the doc here: https://reviews.llvm.org/D89995

Thanks,

19

Just in case somebody misses it, the thread that you're necro-ing here
is 4 years old.

Cheers,
Nicolai