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

Fri Nov 15 08:07:40 PST 2019

Do it.
--paulr

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James Henderson via llvm-dev
Sent: Friday, November 15, 2019 5:17 AM
To: Finkel, Hal J. <hfinkel at anl.gov>
Cc: llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

This all sounds good to me, and reflects certainly how I review and am reviewed. I don't think I have any additional suggestions.

On Fri, 15 Nov 2019 at 03:46, Finkel, Hal J. via llvm-dev <llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>> 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

_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191115/19fa7920/attachment-0001.html>