<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 8, 2016 at 9:52 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Mar 8, 2016 at 9:30 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Mar 8, 2016 at 9:00 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br></span><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>What do you guys think of adding something like this to DeveloperPolicy.rst as an item at the end of the numbered list in <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" target="_blank">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a> ?<br><div><br><div>#. Sometimes patches get committed that need more discussion.</div><div> If a developer thinks that a patch would benefit from some more review</div><div> and promptly communicates this, the patch should be reverted (preferably</div><div> by the original author, unless they are unresponsive).</div><div> Developers often disagree, and erring on the side of the developer</div><div> asking for more review prevents any lingering disagreement over code in</div><div> the tree.</div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div></span><div>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).</div><div>An RFC also increases awareness of the patch which may attract reviewers or accelerate review.</div><div><br></div><div>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.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> Would the late request like this come as a surprise to the developer? </div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div><div>For clarity:</div><div>s/Generally I feel like there is/Generally I feel like there should be/</div><div><br></div><div>-- Sean Silva</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>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.</div><div>I think the situation of r260488 would have fallen under this category.</div><span class=""><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>thanks,</div><div><br></div><div>David</div><span><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>"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.</div><div><br></div><div>"unresponsive" add some nebulousness, but I think it's an important exception to call out for the "preferably by the original author".<span><font color="#888888"><br></font></span></div><span><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span></div>
</blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>