<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body>
<p>Seems reasonable to me. I'm not strongly in favor, but since I
was strongly opposed to the previous proposal, a "don't object"
seemed reasonable to share.</p>
<p>Philip<br>
</p>
<div class="moz-cite-prefix">On 5/17/21 11:12 AM, Krzysztof
Parzyszek via llvm-dev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:BL0PR02MB3748551321F4D4B988497FB0DD2D9@BL0PR02MB3748.namprd02.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style>@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;}a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0in;
font-size:10.0pt;
font-family:Consolas;}span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:Consolas;}.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}div.WordSection1
{page:WordSection1;}ol
{margin-bottom:0in;}ul
{margin-bottom:0in;}</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="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">This
is a revision of the previous RFC[1]. This RFC limits the
scope to pre-commit reviews only.<o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Statement:<o:p></o:p></span></b></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Our
current code review policy states[2]:<o:p></o:p></span></p>
<p class="MsoPlainText" style="margin-left:.5in"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">“Code
reviews are conducted, in order of preference, on our
web-based code-review tool (see Code Reviews with
Phabricator), by email on the relevant project’s commit
mailing list, on the project’s development list, or on the
bug tracker.”<o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">This
proposal is to limit pre-commit code reviews only to
Phabricator. This would apply to all projects in the LLVM
monorepo. With the change in effect, the amended policy
would read:<o:p></o:p></span></p>
<p class="MsoPlainText" style="margin-left:.5in"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">“Pre-commit
code reviews are conducted on our web-based code-review tool
(see Code Reviews with Phabricator). Post-commit reviews
are conducted, in order of preference, on Phabricator, by
email on the relevant project’s commit mailing list, on the
project’s development list, or on the bug tracker.”<o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Current
situation:<o:p></o:p></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l2 level1 lfo1"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">In a
recent llvm-dev thread[3], Christian Kühnel pointed out
that pre-commit code reviews rarely originate via an email
(most are started on Phabricator), although, as others
pointed out, email responses to an ongoing review are not
uncommon. (That thread also contains examples of mishaps
related to the email-Phabricator interactions, or email
handling itself.)<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l2 level1 lfo1"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">We
have Phabricator patches that automatically apply email
comments to the Phabricator reviews, although reportedly
this functionality is not fully reliable[4,5]. This can
cause review comments to be lost in the email traffic.<o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Benefits:<o:p></o:p></span></b></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Single
way of doing pre-commit code reviews: these code reviews
are a key part of the development process, and having one
way of performing them would make the process clearer and
unambiguous.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Review
authors and reviewers would only need to monitor one
source of comments without the fear that a review comment
may end up overlooked.<o:p></o:p></span></li>
<li class="MsoPlainText" style="mso-list:l1 level1 lfo2"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">This
change</span><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
simply codifies an existing practice.</span><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Concerns:</span></b><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p></o:p></span></p>
<ol style="margin-top:0in" type="1" start="1">
<li class="MsoPlainText" style="mso-list:l0 level1 lfo5"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">Because
of the larger variety, email clients may offer better
accessibility options than web browsers.</span><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">
<o:p></o:p></span></li>
</ol>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[1]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[2]
<a
href="https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review"
moz-do-not-send="true">
https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[3]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[4]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif">[5]
<a
href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html"
moz-do-not-send="true">https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html</a><o:p></o:p></span></p>
<p class="MsoPlainText"><span
style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p> </o:p></span></p>
<p class="MsoPlainText"><o:p> </o:p></p>
<p class="MsoPlainText">-- <o:p></o:p></p>
<p class="MsoPlainText">Krzysztof Parzyszek <a
href="mailto:kparzysz@quicinc.com" moz-do-not-send="true">
kparzysz@quicinc.com</a> AI tools development<o:p></o:p></p>
<p class="MsoPlainText"><o:p> </o:p></p>
</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>
</body>
</html>