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

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 <mailto: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.
>     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 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/20191202/3f7220ce/attachment.html>

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>

I have seen a reviewer mark the review as “changes required” and an author mark
it as “changes planned.”
I haven’t seen these used very often, and only for something pretty significant.
As it’s not common practice I’d be reluctant to try to codify it in a policy.
--paulr

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Sean
Silva via llvm-dev
Sent: Wednesday, December 4, 2019 9:12 PM
To: Philip Reames <listmail at philipreames.com>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update



On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <llvm-dev at
lists.llvm.org<mailto: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<mailto: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 list

llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>

https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto: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/20191205/da80ef52/attachment-0001.html>