[llvm-dev] [RFC] High-Level Code-Review Documentation Update

[Kit Barton via llvm-dev]

Hi Hal,
I think this is great! I'm happy to help with this if you need a hand.
At the very least I'm happy to be a reviewer :)

Chris Bieneman and I did a tutorial at the dev conference in October on
Contributing to LLVM and we covered many of these points during the
tutorial. It might be good to add a link to that tutorial as part of
this documentation once it's posted. 

A corollary to your point #1 below - if you are not overly familiar with
some area of the compiler your review is still definitely valuable.
However, it's best not to approve the patch as you may not be familiar
with how the patch integrates with the existing code, etc.

Another question that was asked during the tutorial was how to identify
reviewers for your patches, so expanding our documentation on that might
be good also.

Kit

"Finkel, Hal J. via llvm-dev" <llvm-dev at lists.llvm.org> writes:

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