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

[Finkel, Hal J. via llvm-dev]

On 12/5/19 8:50 AM, Robinson, Paul via llvm-dev wrote:
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.


The best practice I've seen, and one that I'll suggest we document, is that if a new patch does not address all outstanding feedback, the author should explicitly state that in the patch description.

  -Hal

--paulr

From: llvm-dev <llvm-dev-bounces at lists.llvm.org><mailto: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><mailto:listmail at philipreames.com>
Cc: llvm-dev at lists.llvm.org<mailto: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



_______________________________________________
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


--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191205/0b4705fc/attachment.html>