<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>