<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">+ 1. Thanks for writing this up Hal.<div class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Nov 15, 2019, at 2:16 AM, James Henderson via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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 class=""></div><br class=""><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" class="">llvm-dev@lists.llvm.org</a>> wrote:<br class=""></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 class="">
<br class="">
I've been fielding an increasing number of questions about how our <br class="">
code-review process in LLVM works from people who are new to our <br class="">
community, and it's been pointed out to me that our documentation on <br class="">
code reviews is both out of date and not as helpful as it could be to <br class="">
new developers.<br class="">
<br class="">
<a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" rel="noreferrer" target="_blank" class="">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a><br class="">
<br class="">
I would like to compose a patch to update this, but before I do that, I <br class="">
want to highlight some of my thoughts to get feedback. My intent is to <br class="">
capture our community best practices in writing so that people new to <br class="">
our community understand our processes and expectations. Here are some <br class="">
things that I would like to capture:<br class="">
<br class="">
1. You do not need to be an expert in some area of the compiler to <br class="">
review patches; it's fine to ask questions about what some piece of code <br class="">
is doing. If it's not clear to you what is going on, you're unlikely to <br class="">
be the only one. Extra comments and/or test cases can often help (and <br class="">
asking for comments in the test cases is fine as well).<br class="">
<br class="">
2. If you review a patch, but don't intend for the review process to <br class="">
block on your approval, please state that explicitly. Out of courtesy, <br class="">
we generally wait on committing a patch until all reviewers are <br class="">
satisfied, and if you don't intend to look at the patch again in a <br class="">
timely fashion, please communicate that fact in the review.<br class="">
<br class="">
3. All comments by reviewers should be addressed by the patch author. <br class="">
It is generally expected that suggested changes will be incorporated <br class="">
into the next revision of the patch unless the author and/or other <br class="">
reviewers can articulate a good reason to do otherwise (and then the <br class="">
reviewers must agree). If you suggest changes in a code review, but <br class="">
don't wish the suggestion to be interpreted this strongly, please state <br class="">
so explicitly.<br class="">
<br class="">
4. Reviewers may request certain aspects of a patch to be broken out <br class="">
into separate patches for independent review, and also, reviewers may <br class="">
accept a patch conditioned on the author providing a follow-up patch <br class="">
addressing some particular issue or concern (although no committed patch <br class="">
should leave the project in a broken state). Reviewers can also accept a <br class="">
patch conditioned on the author applying some set of minor updates prior <br class="">
to committing, and when applicable, it is polite for reviewers to do so.<br class="">
<br class="">
5. Aim to limit the number of iterations in the review process. For <br class="">
example, when suggesting a change, if you want the author to make a <br class="">
similar set of changes at other places in the code, please explain the <br class="">
requested set of changes so that the author can make all of the changes <br class="">
at once. If a patch will require multiple steps prior to approval (e.g., <br class="">
splitting, refactoring, posting data from specific performance tests), <br class="">
please explain as many of these up front as possible. This allows the <br class="">
patch author to make the most-efficient use of his or her time.<br class="">
<br class="">
6. Some changes are too large for just a code review. Changes that <br class="">
should change the Language Reference (e.g., adding new <br class="">
target-independent intrinsics), adding language extensions in Clang, and <br class="">
so on, require an RFC on *-dev first. For changes that promise <br class="">
significant impact on users and/or downstream code bases, reviewers can <br class="">
request an RFC (Request for Comment) achieving consensus before <br class="">
proceeding with code review. That having been said, posting initial <br class="">
patches can help with discussions on an RFC.<br class="">
<br class="">
Lastly, the current text reads, "Code reviews are conducted by email on <br class="">
the relevant project’s commit mailing list, or alternatively on the <br class="">
project’s development list or bug tracker.", and then only later <br class="">
mentions Phabricator. I'd like to move Phabricator to be mentioned on <br class="">
this line before the other methods.<br class="">
<br class="">
Please let me know what you think.<br class="">
<br class="">
Thanks again,<br class="">
<br class="">
Hal<br class="">
<br class="">
-- <br class="">
Hal Finkel<br class="">
Lead, Compiler Technology and Programming Languages<br class="">
Leadership Computing Facility<br class="">
Argonne National Laboratory<br class="">
<br class="">
_______________________________________________<br class="">
LLVM Developers mailing list<br class="">
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a><br class="">
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br class="">
</blockquote></div>
_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" class="">llvm-dev@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<br class=""></div></blockquote></div><br class=""></div></body></html>