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

[Philip Reames via llvm-dev]

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>