Sean Silva via llvm-dev
2016-Mar-09 05:00 UTC
[llvm-dev] Formalize "revert for more design review" policy.
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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/3cf2d2d4/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: revertpolicy.patch Type: application/octet-stream Size: 1181 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/3cf2d2d4/attachment.obj>
Sean Silva via llvm-dev
2016-Mar-09 05:03 UTC
[llvm-dev] Formalize "revert for more design review" policy.
+Hal, who somehow slipped through the cracks. On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <chisophugis at gmail.com> wrote:> 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/64aca344/attachment.html>
Hal Finkel via llvm-dev
2016-Mar-09 05:13 UTC
[llvm-dev] Formalize "revert for more design review" policy.
----- Original Message -----> From: "Sean Silva" <chisophugis at gmail.com> > To: "llvm-dev" <llvm-dev at lists.llvm.org> > Cc: "Chris Lattner" <clattner at apple.com>, "Rafael Ávila de Espíndola" <rafael.espindola at gmail.com>, "Michael Spencer" > <bigcheesegs at gmail.com>, "Chandler Carruth" <chandlerc at gmail.com>, "David Li" <davidxl at google.com>, "David Blaikie" > <dblaikie at gmail.com>, "Daniel Berlin" <dberlin at dberlin.org>, "Easwaran Raman" <eraman at google.com>, "Hal Finkel" > <hfinkel at 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 :-)> > On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva < chisophugis at gmail.com > > wrote: > > > > 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> > > -- Sean Silva >-- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory
Xinliang David Li via llvm-dev
2016-Mar-09 05:30 UTC
[llvm-dev] Formalize "revert for more design review" policy.
On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <chisophugis at gmail.com> wrote:> 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> > "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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/97214a1b/attachment.html>
Mehdi Amini via llvm-dev
2016-Mar-09 05:35 UTC
[llvm-dev] Formalize "revert for more design review" policy.
> On Mar 8, 2016, at 9:30 PM, Xinliang David Li via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > > > On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <chisophugis at gmail.com <mailto:chisophugis at gmail.com>> wrote: > 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 <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. FWIW I like Sean proposal, and I agree with David about the "priority increase" glitch. -- Mehdi> > thanks, > > David > > > > > > "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 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://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/20160308/6968cdc9/attachment.html>
Sean Silva via llvm-dev
2016-Mar-09 05:52 UTC
[llvm-dev] Formalize "revert for more design review" policy.
On Tue, Mar 8, 2016 at 9:30 PM, Xinliang David Li <davidxl at google.com> wrote:> > > On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <chisophugis at gmail.com> wrote: > >> 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> > thanks, > > David > > > > > >> >> "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 >> > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160308/71865eca/attachment.html>
Philip Reames via llvm-dev
2016-Mar-09 17:37 UTC
[llvm-dev] Formalize "revert for more design review" policy.
On 03/08/2016 09:30 PM, Xinliang David Li via llvm-dev wrote:> > > On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <chisophugis at gmail.com > <mailto:chisophugis at gmail.com>> wrote: > > 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.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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160309/5fe443ec/attachment-0001.html>
Philip Reames via llvm-dev
2016-Mar-09 17:43 UTC
[llvm-dev] Formalize "revert for more design review" policy.
On 03/08/2016 09:00 PM, Sean Silva via llvm-dev wrote:> 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.+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."> > "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 > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://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/20160309/53d33c62/attachment.html>
Renato Golin via llvm-dev
2016-Mar-11 15:46 UTC
[llvm-dev] Formalize "revert for more design review" policy.
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 On 9 March 2016 at 12:00, Sean Silva via llvm-dev <llvm-dev at lists.llvm.org> wrote:> 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 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >
Mehdi AMINI via llvm-dev
2020-Oct-22 23:37 UTC
[llvm-dev] Formalize "revert for more design review" policy.
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, -- Mehdi On Fri, Mar 11, 2016 at 7:46 AM Renato Golin via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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 > > On 9 March 2016 at 12:00, Sean Silva via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > 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 > > > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://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/20201022/5fd22ffc/attachment.html>