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

Mon Dec 2 07:55:52 PST 2019

Yeah, +1 that people from the same organization are sometimes the only ones
working on a certain feature/area. (certainly I'd expect some discussion
about the feature in general to be discussed outside that group if it's in
any way contentious - but some stuff's clear enough (I think I implemented
debug_types years ago, likely with only Eric's approval, both of us being
at Google (probably many DWARF features were added/done this way, to be
honest - maybe some could've done witha  bit of broader discussion, but I
don't think either of us were "rubber stamping" the other's work (if
anything I'm harder on my "friends" to be honest... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

>
>
> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>> +1 in general, and Philip has good suggestions as well!
>>
>> --
>> Mehdi
>>
>>
>> On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> + 1 in general, a couple of suggestions
>>>
>>> On 11/14/19 7:46 PM, 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:
>>> >
>>> >   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).
>>> Authors are encouraged to interpret questions as reasons to reexamine
>>> the readability of the code in question.  Structural changes, or further
>>> comments may be appropriate.
>>> >
>>> >   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.
>>> If the path forward is not clear - because the patch is too large to
>>> meaningful review, or direction needs to be settled - it is fine to
>>> suggest a clear next step (e.g. landing a refactoring) followed by a
>>> re-review.  Please state explicitly if the path forward is unclear to
>>> prevent confusions on the part of the author.
>>> >
>>> >   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
>>>
>>> A couple of additional things:
>>>
>>> Only a single LGTM is required.  Reviewers are expected to only LGTM
>>> patches they're confident in their knowledge of.  Reviewers may review
>>> and provide suggestions, but explicitly defer LGTM to someone else.
>>> This is encouraged and a good way for new contributors to learn the
>>> code.
>>>
>>> There is a cultural expectation that at least one reviewer is from a
>>> different organization than the author of the patch.
>>
>>
> Actually, while I'm OK with the other suggestions, I didn't pay attention
> to this one originally.
> I'm very concerned about this: this looks like an assumption of bad faith
> or malice in the review process, and I find this unhealthy if it were part
> of the "cultural expectation". Moreover there are many areas of the
> compiler where there aren't many people available to review changes.
>
> I personally never really paid attention to who is the author/reviewer of
> a patch from an organizational point of view, I haven't perceived this
> culture of looking into affiliation so far. I never got the impression that
> reviewer were more difficult with me than they would be with others.
> There have been many patches that I reviewed that originated from other
> people from the same company as mine (back when I was at Apple mostly). The
> notion of "organization" is blurry: frequently this involved people from
> different teams inside the same company,  are they part of "the same
> organization"? Some of these people I have never even ever met or never
> heard of them before reviewing a patch (sometimes I don't even realize
> since there is a Phabricator pseudo and not everyone is using their
> business email here).
>
> --
> Mehdi
>
>
>
>
>
>> If that's not
>>> possible, care should be taken to ensure overall direction has been
>>> widely accepted.
>>>
>>> Post commit review is encouraged via either phabricator or email.  There
>>> is a strong expectation that authors respond promptly to post commit
>>> feedback and address it.  Failure to do so is cause for the patch to be
>>> reverted.  If substantial problems are identified, it is expected that
>>> the patch is reverted, fixed offline, and then recommitted (possibly
>>> after further review.)
>>>
>>>
>>> _______________________________________________
>>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/2a360f09/attachment-0001.html>