<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">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"><o:p> </o:p></p>
<p class="MsoNormal">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"><o:p> </o:p></p>
<p class="MsoNormal">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. 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>
<p class="MsoNormal">--paulr<o:p></o:p></p>
<p class="MsoNormal"><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 <llvm-dev-bounces@lists.llvm.org> <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 <joker.eph@gmail.com><br>
<b>Cc:</b> 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>
<p class="MsoNormal">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"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">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>
<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 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-right:0in">
<div>
<p class="MsoNormal">+1 in general, and Philip has good suggestions as well!<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">Mehdi<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">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-right:0in">
<p class="MsoNormal">+ 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"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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">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"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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">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"><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>
<div>
<p class="MsoNormal"><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>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<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 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">_______________________________________________<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>
</body>
</html>