<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
p.gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext, li.gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext, div.gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext
{mso-style-name:gmail-m_4065269081265090465gmail-m8924233148030043466msoplaintext;
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.EmailStyle19
{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;}
/* List Definitions */
@list l0
{mso-list-id:664673625;
mso-list-template-ids:1147950678;}
@list l1
{mso-list-id:854852676;
mso-list-template-ids:-1542192448;}
@list l2
{mso-list-id:1129974424;
mso-list-template-ids:1110189008;}
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]-->
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal">Code review guidelines patch is available for review: <a href="https://reviews.llvm.org/D103811">
https://reviews.llvm.org/D103811</a>.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:Consolas">-- </span>
<span style="font-size:9.0pt;font-family:Consolas"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:8.0pt;font-family:Consolas">Krzysztof Parzyszek
<a href="mailto:kparzysz@quicinc.com"><span style="color:#0563C1">kparzysz@quicinc.com</span></a> AI tools development<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> David Blaikie <dblaikie@gmail.com> <br>
<b>Sent:</b> Tuesday, May 18, 2021 3:01 PM<br>
<b>To:</b> Krzysztof Parzyszek <kparzysz@quicinc.com><br>
<b>Cc:</b> llvm-dev <llvm-dev@lists.llvm.org>; clangd-dev@lists.llvm.org; openmp-dev@lists.llvm.org; lldb-dev@lists.llvm.org; cfe-dev@lists.llvm.org; libcxx-dev@lists.llvm.org; flang-dev@lists.llvm.org; parallel_libs-dev@lists.llvm.org<br>
<b>Subject:</b> [EXT] Re: [cfe-dev] [RFC] Deprecate pre-commit email code reviews in favor of Phabricator<o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Tue, May 18, 2021 at 6:50 AM Krzysztof Parzyszek <<a href="mailto:kparzysz@quicinc.com">kparzysz@quicinc.com</a>> wrote:<o:p></o:p></p>
</div>
<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>
<div>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="margin-left:1.0in">
Post-commit reviews are conducted, in order of preference, on Phabricator, <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:.5in">
This still seems like a change in practice that I'm not in favor of, personally - due to the current divergence between email and phab review feedback. Yes, this would be one way to unify it - but I'm not sure it's necessarily the best one.<br>
<br>
I'd suggest leaving this to a separate proposal so as not to complicate/muddy the waters of the formalization of pre-commit review practice.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I simply broke up the existing sentence from the documentation into two parts, one about pre-commit reviews and the other about all other code reviews (which are basically the post-commit
reviews, although I’m open to corrections here). The first part was modified to reflect the proposed change, the second part was left unchanged. <o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
I think the issue is that the original phrasing was probably only intended to describe the preference for pre-commit review. (I think statements about post-commit review could reasonably read to be only those that say "post-commit review", in this ( <a href="https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed">https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed</a>
) section. <br>
<br>
So I think (at least in terms of how to read it in a way that matches existing practice) the original wording amounted to something like this:<br>
<br>
... "post-commit review can use any of the tools listed below" ... <br>
... "pre-commit review is done in this order of phab, email, etc... "<br>
<br>
ie: the post-commit review didn't have the same order of preference as pre-commit review.<br>
<br>
I'd probably pull out the post-commit review-specific wording back up to where post-commit review is discussed, and leave the rest of this to talk about pre-commit review (most of this document discussing unqualified "review" seems predominantly to be talking
about "pre-commit review" except the part that talks about "post commit review").<br>
<br>
Probably move the "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." (without the "in order of preference") up to the "post-commit
review" section, instead of referencing a version of it here.<br>
<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>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">In this RFC I only want to change the part of the documentation that pertains specifically to pre-commit code reviews. If the wording I used creates confusion, what would you suggest
instead?<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:9.0pt;font-family:Consolas">--
</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:8.0pt;font-family:Consolas">Krzysztof Parzyszek
<a href="mailto:kparzysz@quicinc.com" target="_blank"><span style="color:#0563C1">kparzysz@quicinc.com</span></a> AI tools development</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b>From:</b> David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>>
<br>
<b>Sent:</b> Monday, May 17, 2021 4:40 PM<br>
<b>To:</b> Krzysztof Parzyszek <<a href="mailto:kparzysz@quicinc.com" target="_blank">kparzysz@quicinc.com</a>><br>
<b>Cc:</b> llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>;
<a href="mailto:clangd-dev@lists.llvm.org" target="_blank">clangd-dev@lists.llvm.org</a>;
<a href="mailto:openmp-dev@lists.llvm.org" target="_blank">openmp-dev@lists.llvm.org</a>;
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>;
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>;
<a href="mailto:libcxx-dev@lists.llvm.org" target="_blank">libcxx-dev@lists.llvm.org</a>;
<a href="mailto:flang-dev@lists.llvm.org" target="_blank">flang-dev@lists.llvm.org</a>;
<a href="mailto:parallel_libs-dev@lists.llvm.org" target="_blank">parallel_libs-dev@lists.llvm.org</a><br>
<b>Subject:</b> [EXT] Re: [cfe-dev] [RFC] Deprecate pre-commit email code reviews in favor of Phabricator<o:p></o:p></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">On Mon, May 17, 2021 at 11:12 AM Krzysztof Parzyszek via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">This is a revision of the previous RFC[1]. This RFC limits the scope to pre-commit reviews only.<o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"><b>Statement:</b><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">Our current code review policy states[2]:<o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="margin-left:.5in">
“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></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">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></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="margin-left:.5in">
“Pre-commit code reviews are conducted on our web-based code-review tool (see Code Reviews with Phabricator). <o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I'm with you here ^, this seems to document/formalize existing practice - though does this accurately reflect all the projects in the mororepo? I get the impression that mlir, maybe
flang, etc might be doing reviews differently? <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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="margin-left:.5in">
Post-commit reviews are conducted, in order of preference, on Phabricator, <o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">This still seems like a change in practice that I'm not in favor of, personally - due to the current divergence between email and phab review feedback. Yes, this would be one way
to unify it - but I'm not sure it's necessarily the best one.<br>
<br>
I'd suggest leaving this to a separate proposal so as not to complicate/muddy the waters of the formalization of pre-commit review practice.<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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="margin-left:.5in">
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></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"><b>Current situation:</b><o:p></o:p></p>
<ol start="1" type="1">
<li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l0 level1 lfo1">
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></li><li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l0 level1 lfo1">
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></li></ol>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"><b>Benefits:</b><o:p></o:p></p>
<ol start="1" type="1">
<li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l2 level1 lfo2">
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></li><li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l2 level1 lfo2">
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></li><li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l2 level1 lfo2">
This change simply codifies an existing practice.<o:p></o:p></li></ol>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"><b>Concerns:</b><o:p></o:p></p>
<ol start="1" type="1">
<li class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext" style="mso-list:l1 level1 lfo3">
Because of the larger variety, email clients may offer better accessibility options than web browsers.
<o:p></o:p></li></ol>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">[1] <a href="https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html" target="_blank">
https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html</a><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">[2] <a href="https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review" target="_blank">
https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review</a><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">[3] <a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html" target="_blank">
https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html</a><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">[4] <a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html" target="_blank">
https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html</a><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">[5] <a href="https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html" target="_blank">
https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html</a><o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">-- <o:p>
</o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext">Krzysztof Parzyszek
<a href="mailto:kparzysz@quicinc.com" target="_blank">kparzysz@quicinc.com</a> AI tools development<o:p></o:p></p>
<p class="gmail-m4065269081265090465gmail-m8924233148030043466msoplaintext"> <o:p></o:p></p>
</div>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</body>
</html>