<div><br></div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<p><br>
</p>
<div>On 12/2/19 8:52 AM, Hubert Tong via
llvm-dev wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Nov 14, 2019 at
10:46 PM Finkel, Hal J. via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
3. All comments by reviewers should be addressed by the
patch author. <br>
It is generally expected that suggested changes will be
incorporated <br>
into the next revision of the patch unless the author and/or
other <br>
reviewers can articulate a good reason to do otherwise (and
then the <br>
reviewers must agree).<br>
</blockquote>
<div>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.<br>
</div>
</div>
</div>
</blockquote>
<p>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. <br>
</p>
<p>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.
</p></div></blockquote><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">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).</div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">— Sean Silva</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p> <br>
</p></div><div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">If you suggest changes in
a code review, but <br>
don't wish the suggestion to be interpreted this strongly,
please state <br>
so explicitly.<br>
</blockquote>
</div>
</div>
<br>
<fieldset></fieldset>
<pre>_______________________________________________
LLVM Developers mailing list
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
</div>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>