<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 8, 2016 at 9:13 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "Sean Silva" <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>><br>
> To: "llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>><br>
> Cc: "Chris Lattner" <<a href="mailto:clattner@apple.com">clattner@apple.com</a>>, "Rafael Ávila de Espíndola" <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>>, "Michael Spencer"<br>
> <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>>, "David Li" <<a href="mailto:davidxl@google.com">davidxl@google.com</a>>, "David Blaikie"<br>
> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>>, "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>>, "Easwaran Raman" <<a href="mailto:eraman@google.com">eraman@google.com</a>>, "Hal Finkel"<br>
> <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
> Sent: Tuesday, March 8, 2016 11:03:09 PM<br>
> Subject: Re: Formalize "revert for more design review" policy.<br>
><br>
><br>
> +Hal, who somehow slipped through the cracks.<br>
><br>
<br>
</span>Thanks :-)<br>
<span class=""><br>
><br>
> On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva < <a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a> ><br>
> wrote:<br>
><br>
><br>
><br>
> Recently there's been some friction over reversions (I can remember<br>
> two cases in recent memory). In both issues the general feel I got<br>
> is that as a community we should honor "revert for more design<br>
> review" requests unconditionally.<br>
><br>
><br>
> What do you guys think of adding something like this to<br>
> DeveloperPolicy.rst as an item at the end of the numbered list in<br>
> <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" rel="noreferrer" target="_blank">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a> ?<br>
><br>
><br>
><br>
> #. Sometimes patches get committed that need more discussion.<br>
> If a developer thinks that a patch would benefit from some more<br>
> review<br>
> and promptly communicates this, the patch should be reverted<br>
> (preferably<br>
> by the original author, unless they are unresponsive).<br>
> Developers often disagree, and erring on the side of the developer<br>
> asking for more review prevents any lingering disagreement over code<br>
> in<br>
> the tree.<br>
><br>
><br>
> "promptly" is there mostly to avoid suggesting a "necro-revert"; once<br>
> the code has been in tree for long enough at some point it would be<br>
> more appropriate to open a bug report or start a fresh discussion.<br>
><br>
><br>
> "unresponsive" add some nebulousness, but I think it's an important<br>
> exception to call out for the "preferably by the original author".<br>
><br>
<br>
</span>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.<br></blockquote><div><br></div><div>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".<br><br>Committing prematurely shouldn't be a way to cause a priority increase for a patch.<br><br>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.<br><br>(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))</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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).<br></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 -Hal<br>
<br>
><br>
><br>
> -- Sean Silva<br>
><br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>