<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<div class="moz-cite-prefix">On 12/5/19 8:50 AM, Robinson, Paul via llvm-dev wrote:<br>
</div>
<blockquote type="cite" cite="mid:BYAPR13MB2421A724FDCF590F38926D72925C0@BYAPR13MB2421.namprd13.prod.outlook.com">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Consolas",serif;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal">I have seen a reviewer mark the review as “changes required” and an author mark it as “changes planned.”<o:p></o:p></p>
<p class="MsoNormal">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.</p>
</div>
</blockquote>
<p><br>
</p>
<p>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.</p>
<p> -Hal<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:BYAPR13MB2421A724FDCF590F38926D72925C0@BYAPR13MB2421.namprd13.prod.outlook.com">
<div class="WordSection1">
<p class="MsoNormal"><o:p></o:p></p>
<p class="MsoNormal">--paulr<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in
0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <a class="moz-txt-link-rfc2396E" href="mailto:llvm-dev-bounces@lists.llvm.org">
<llvm-dev-bounces@lists.llvm.org></a> <b>On Behalf Of </b>Sean Silva via llvm-dev<br>
<b>Sent:</b> Wednesday, December 4, 2019 9:12 PM<br>
<b>To:</b> Philip Reames <a class="moz-txt-link-rfc2396E" href="mailto:listmail@philipreames.com">
<listmail@philipreames.com></a><br>
<b>Cc:</b> <a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">
llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC
1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p><o:p> </o:p></p>
<div>
<p class="MsoNormal">On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<p class="MsoNormal">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" moz-do-not-send="true">llvm-dev@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid
#CCCCCC 1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal"> 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).<o:p></o:p></p>
</blockquote>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</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.
<o:p></o:p></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.
<o:p></o:p></p>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">— Sean Silva<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC
1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p><o:p> </o:p></p>
</div>
<div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid
#CCCCCC 1.0pt;padding:0in 0in 0in
6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">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.<o:p></o:p></p>
</blockquote>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<pre>_______________________________________________<o:p></o:p></pre>
<pre>LLVM Developers mailing list<o:p></o:p></pre>
<pre><a href="mailto:llvm-dev@lists.llvm.org" target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><o:p></o:p></pre>
<pre><a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></pre>
</blockquote>
</div>
<p class="MsoNormal">_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" moz-do-not-send="true">llvm-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
LLVM Developers mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
</blockquote>
<pre class="moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</body>
</html>