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

Sat Nov 16 08:14:39 PST 2019

+1, looks good to me.

On Sat, Nov 16, 2019 at 7:12 PM David Blaikie via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> Sounds generally accurate & good to me.
>
> On Fri, Nov 15, 2019 at 2:17 AM James Henderson via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>>
>> 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> 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
Roman.

>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev