[llvm-dev] [RFC] High-Level Code-Review Documentation Update
Finkel, Hal J. via llvm-dev
llvm-dev at lists.llvm.org
Thu Dec 26 14:45:56 PST 2019
I would like to thank everyone who provided feedback on this thread. I
attempted to incorporate all of it. If I missed something, please let me
know.
I've posted a patch to our documentation here:
https://reviews.llvm.org/D71916
-Hal
On 11/14/19 9:45 PM, Hal Finkel 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:
>
> 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
>
--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-dev
mailing list