[llvm-dev] [RFC] High-Level Code-Review Documentation Update

[Sean Silva via llvm-dev]

On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

>
> On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:
>
> On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>>   3. All comments by reviewers should be addressed by the patch author.
>> It is generally expected that suggested changes will be incorporated
>> into the next revision of the patch unless the author and/or other
>> reviewers can articulate a good reason to do otherwise (and then the
>> reviewers must agree).
>>
> I disagree on the high bar here. The author should acknowledge the
> comments; however, addressing all of the comments in one shot has similar
> problems as having commits that are too large (diffs between revisions
> become more difficult to review). This also leads to significant timing
> issues, where the comments made overnight in some time zone are addressed
> by the author locally, but someone added comments in the afternoon the next
> day before the author has a chance to post the new revision.
>
> Then the author needs to indicate this explicitly, and except in
> exceptional circumstances with an explicit request, should not expect
> re-review until comments are addressed.  It's fine to post a new version of
> the patch; just not to expect action by reviewers.
>
> I see this one as a major stumbling block for new contributors - i.e.
> reviews get stuck because both sides expect the other to be doing
> something.  Having it clearly documented is important IMO.
>


Does phabricator has some sort of functionality to formalize this? I feel
like I’ve seen that somewhere on phab (it wasn’t the llvm phab instance
though).

One of the code review systems I’m using these days has a feature that a
patch stays “pending” until not only LGTM is given, but also all comments
have been marked as acknowledged/resolved in the UI, at which point the web
UI turns green and is “ready” to submit.

— Sean Silva

>
>
>
>> If you suggest changes in a code review, but
>> don't wish the suggestion to be interpreted this strongly, please state
>> so explicitly.
>>
>
> _______________________________________________
> LLVM Developers mailing listllvm-dev at lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191204/05e1acf1/attachment.html>