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

[Sean Silva via llvm-dev]

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>