Robinson, Paul via llvm-dev
2019-Dec-05 14:50 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
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>
Finkel, Hal J. via llvm-dev
2019-Dec-05 15:35 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
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>
Renato Golin via llvm-dev
2019-Dec-09 13:33 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
On Thu, 5 Dec 2019 at 15:35, Finkel, Hal J. via llvm-dev <llvm-dev at lists.llvm.org> wrote:> 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.+1! For example: to be addressed by a following patch, or assumption no longer valid, etc.