<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"David Blaikie" <dblaikie@gmail.com><br><b>To: </b>"Hal Finkel" <hfinkel@anl.gov><br><b>Cc: </b>"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><br><b>Sent: </b>Tuesday, March 8, 2016 11:22:11 PM<br><b>Subject: </b>Re: Formalize "revert for more design review" policy.<br><br><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: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><span class=""><hr id="zwchr"><br>
> From: "Sean Silva" <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>><br>
> To: "llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
> Cc: "Chris Lattner" <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>>, "Rafael Ávila de Espíndola" <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>>, "Michael Spencer"<br>
> <<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>>, "David Li" <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>>, "David Blaikie"<br>
> <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>>, "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>>, "Easwaran Raman" <<a href="mailto:eraman@google.com" target="_blank">eraman@google.com</a>>, "Hal Finkel"<br>
> <<a href="mailto:hfinkel@anl.gov" target="_blank">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" target="_blank">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 id="DWT2661">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></div></div></div></div></blockquote><br>Completely agree. Although that does not fall into the "revert for more design review" category.<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div id="DWT2662"><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></div></blockquote>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.<br><br>Thanks again,<br>Hal<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); 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: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); 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>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>