<div dir="ltr">This all sounds good to me, and reflects certainly how I review and am reviewed. I don't think I have any additional suggestions.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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" rel="noreferrer" 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>
<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>
<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>
-- <br>
Hal Finkel<br>
Lead, Compiler Technology and Programming Languages<br>
Leadership Computing Facility<br>
Argonne National Laboratory<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" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div>