<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;}
/* 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;}
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.EmailStyle18
        {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]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Regarding the talk I heard, Mehdi asked:<o:p></o:p></p>
<p class="MsoNormal" style="margin-left:.5in">The message was maybe not convey clearly?<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">See â€œModern Code Review: A Case Study at Google,” by Caitlin Sadowski et al, 40<sup>th</sup> ICSE (2018), DOI: 10.1145/3183519.3183525<o:p></o:p></p>
<p class="MsoNormal">In particular, Finding 1 says that readability/maintainability is the primary focus. Defect detection is clearly secondary.<o:p></o:p></p>
<p class="MsoNormal">I spoke to Caitlin after the paper presentation to verify this point.  But she said readability really is the primary goal, not defect detection.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">If the paper does not reflect your perception of how review actually works, well, that may be a problem with the paper, or perhaps practices vary across the company; but I suggest you check with Caitlin or one of her co-authors first. 
 I can only report what Google authors reported.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">In any case, the link you cited is a nice, short list and a worthwhile starting point for a set of LLVM code-review goals.<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> Mehdi AMINI <joker.eph@gmail.com> <br>
<b>Sent:</b> Monday, December 2, 2019 11:49 AM<br>
<b>To:</b> Robinson, Paul <paul.robinson@sony.com><br>
<b>Cc:</b> David Blaikie <dblaikie@gmail.com>; llvm-dev@lists.llvm.org<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>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Dec 2, 2019 at 8:36 AM Robinson, Paul <<a href="mailto:paul.robinson@sony.com">paul.robinson@sony.com</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>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">yes, I remember marveling at how dblaikie and echristo would have occasionally boisterous debates on-list when they essentially (possibly literally) sat next to each other.<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">When my team started participating upstream, we were advised to try to avoid getting approvals from each other, and specifically seek out reviewers from other orgs.  Partly this
 is a matter of perspective, in that what’s good for one org and what’s good for the upstream project may be fairly different, and that may be clearer to someone from a different org.  I also think if a patch goes through an internal review step before being
 posted upstream (a process we employ a fair amount in my team) then it’s best if someone else does the upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s necessary to codify this too strongly, it’s more of a how-to-pick-reviewers
 topic than how-we-do-reviews.<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">It would be more helpful if the Code Reviews section stated a goal (or short goal set) for the review.  People can have radically different ideas about the purpose of a code review. 
 I attended a software-engineering conference talk once where Google people stated flat out that the sole purpose of their internal reviews is readability. <o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">The message was maybe not convey clearly? Google's internal review require two kinds of approval and one of them is purely for "readability", and not everyone can give this approval (you need to get enough of your own patches through first).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">In my experience reviews are fairly aligned with Google's public documentation on the topic: <a href="https://google.github.io/eng-practices/review/">https://google.github.io/eng-practices/review/</a> (which I find to be a pretty good document
 by the way).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">-- <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Mehdi<o:p></o:p></p>
</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">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Readability is nice, it fosters maintainability which is certainly a good thing; but it goes against my entire career’s notion that reviews primarily seek out logic holes and defects. <o:p></o:p></p>
</div>
</div>
</blockquote>
<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">--paulr<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>
<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" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b>From:</b> llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>>
<b>On Behalf Of </b>David Blaikie via llvm-dev<br>
<b>Sent:</b> Monday, December 2, 2019 10:56 AM<br>
<b>To:</b> Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>><br>
<b>Cc:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">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" 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">Yeah, +1 that people from the same organization are sometimes the only ones working on a certain feature/area. (certainly I'd expect some discussion about the feature in general
 to be discussed outside that group if it's in any way contentious - but some stuff's clear enough (I think I implemented debug_types years ago, likely with only Eric's approval, both of us being at Google (probably many DWARF features were added/done this
 way, to be honest - maybe some could've done witha  bit of broader discussion, but I don't think either of us were "rubber stamping" the other's work (if anything I'm harder on my "friends" to be honest... :/ )))<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 Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<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 Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</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>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">+1 in general, and Philip has good suggestions as well!<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>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Mehdi<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>
</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 Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">+ 1 in general, a couple of suggestions<br>
<br>
On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:<br>
> Hi, everyone,<br>
><br>
> I've been fielding an increasing number of questions about how our <br>
> code-review process in LLVM works from people who are new to our <br>
> community, and it's been pointed out to me that our documentation on <br>
> code reviews is both out of date and not as helpful as it could be to <br>
> new developers.<br>
><br>
>    <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" target="_blank">
http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a><br>
><br>
> I would like to compose a patch to update this, but before I do that, I <br>
> want to highlight some of my thoughts to get feedback. My intent is to <br>
> capture our community best practices in writing so that people new to <br>
> our community understand our processes and expectations. Here are some <br>
> things that I would like to capture:<br>
><br>
>   1. You do not need to be an expert in some area of the compiler to <br>
> review patches; it's fine to ask questions about what some piece of code <br>
> is doing. If it's not clear to you what is going on, you're unlikely to <br>
> be the only one. Extra comments and/or test cases can often help (and <br>
> asking for comments in the test cases is fine as well).<br>
Authors are encouraged to interpret questions as reasons to reexamine<br>
the readability of the code in question.  Structural changes, or further<br>
comments may be appropriate.<br>
><br>
>   2. If you review a patch, but don't intend for the review process to <br>
> block on your approval, please state that explicitly. Out of courtesy, <br>
> we generally wait on committing a patch until all reviewers are <br>
> satisfied, and if you don't intend to look at the patch again in a <br>
> timely fashion, please communicate that fact in the review.<br>
><br>
>   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). 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>
><br>
>   4. Reviewers may request certain aspects of a patch to be broken out <br>
> into separate patches for independent review, and also, reviewers may <br>
> accept a patch conditioned on the author providing a follow-up patch <br>
> addressing some particular issue or concern (although no committed patch <br>
> should leave the project in a broken state). Reviewers can also accept a <br>
> patch conditioned on the author applying some set of minor updates prior <br>
> to committing, and when applicable, it is polite for reviewers to do so.<br>
><br>
>   5. Aim to limit the number of iterations in the review process. For <br>
> example, when suggesting a change, if you want the author to make a <br>
> similar set of changes at other places in the code, please explain the <br>
> requested set of changes so that the author can make all of the changes <br>
> at once. If a patch will require multiple steps prior to approval (e.g., <br>
> splitting, refactoring, posting data from specific performance tests), <br>
> please explain as many of these up front as possible. This allows the <br>
> patch author to make the most-efficient use of his or her time.<br>
If the path forward is not clear - because the patch is too large to<br>
meaningful review, or direction needs to be settled - it is fine to<br>
suggest a clear next step (e.g. landing a refactoring) followed by a<br>
re-review.  Please state explicitly if the path forward is unclear to<br>
prevent confusions on the part of the author. <br>
><br>
>   6. Some changes are too large for just a code review. Changes that <br>
> should change the Language Reference (e.g., adding new <br>
> target-independent intrinsics), adding language extensions in Clang, and <br>
> so on, require an RFC on *-dev first. For changes that promise <br>
> significant impact on users and/or downstream code bases, reviewers can <br>
> request an RFC (Request for Comment) achieving consensus before <br>
> proceeding with code review. That having been said, posting initial <br>
> patches can help with discussions on an RFC.<br>
><br>
> Lastly, the current text reads, "Code reviews are conducted by email on <br>
> the relevant project’s commit mailing list, or alternatively on the <br>
> project’s development list or bug tracker.", and then only later <br>
> mentions Phabricator. I'd like to move Phabricator to be mentioned on <br>
> this line before the other methods.<br>
><br>
> Please let me know what you think.<br>
><br>
> Thanks again,<br>
><br>
> Hal<br>
<br>
A couple of additional things:<br>
<br>
Only a single LGTM is required.  Reviewers are expected to only LGTM<br>
patches they're confident in their knowledge of.  Reviewers may review<br>
and provide suggestions, but explicitly defer LGTM to someone else. <br>
This is encouraged and a good way for new contributors to learn the code. <br>
<br>
There is a cultural expectation that at least one reviewer is from a<br>
different organization than the author of the patch. <o:p></o:p></p>
</blockquote>
</div>
</blockquote>
<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">Actually, while I'm OK with the other suggestions, I didn't pay attention to this one originally.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I'm very concerned about this: this looks like an assumption of bad faith or malice in the review process, and I find this unhealthy if it were part of the "cultural expectation".
 Moreover there are many areas of the compiler where there aren't many people available to review changes.<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>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I personally never really paid attention to who is the author/reviewer of a patch from an organizational point of view, I haven't perceived this culture of looking into affiliation
 so far. I never got the impression that reviewer were more difficult with me than they would be with others.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">There have been many patches that I reviewed that originated from other people from the same company as mine (back when I was at Apple mostly). The notion of "organization" is blurry:
 frequently this involved people from different teams inside the same company,  are they part of "the same organization"? Some of these people I have never even ever met or never heard of them before reviewing a patch (sometimes I don't even realize since there
 is a Phabricator pseudo and not everyone is using their business email here).<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>
<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">Mehdi<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>
<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>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <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>
<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">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">If that's not<br>
possible, care should be taken to ensure overall direction has been<br>
widely accepted. <br>
<br>
Post commit review is encouraged via either phabricator or email.  There<br>
is a strong expectation that authors respond promptly to post commit<br>
feedback and address it.  Failure to do so is cause for the patch to be<br>
reverted.  If substantial problems are identified, it is expected that<br>
the patch is reverted, fixed offline, and then recommitted (possibly<br>
after further review.)<br>
<br>
<br>
_______________________________________________<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" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></p>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">_______________________________________________<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" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</body>
</html>