[llvm-dev] Formalize "revert for more design review" policy.

[Sean Silva via llvm-dev]

On Tue, Mar 8, 2016 at 9:52 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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.
>

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

-- Sean Silva


> 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/675a838d/attachment-0001.html>