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

[Doerfert, Johannes via llvm-dev]

+1, more below

On 11/15, Finkel, Hal J. via llvm-dev wrote:
> 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:

This is great. There is often uncertainty in the review process (I hear
a lot "I cannot review/accept this"), and there are often review
processes that can be improved (I hear complaints about delays and other
things).


>   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).

Fully agreed, especially with 2. in place.


>   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.

Fully agreed.


>   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.

I like this together with 4.

>   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.

I'd even encourage the second part of this. I would like to see faster
turn-around times by getting good patches in that on which follow-ups
are applied (to some degree at least).


>   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.

This is one that is really important to me. Again, I would go further.
Reviewers that want to be blocking reviewers should look at the whole
patch, to the degree feasible, and not only certain area without leaving
a reason. It is not healthy if we get 10 review rounds with two minor
comments each.


>   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.

Agreed.


> 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.

Yes, please.


> Please let me know what you think.

We should definitively improve our documentation, maybe with links to
outside sources or based on them. I read [0] recently which has a lot of
really good points. Now I am actively trying to improve my reviews based
on that.

[0] https://google.github.io/eng-practices/review/reviewer/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191115/2819289d/attachment-0001.sig>