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

Mon Dec 2 09:48:51 PST 2019

On Mon, Dec 2, 2019 at 11:36 AM Robinson, Paul <paul.robinson at sony.com>
wrote:

> yes, I remember marveling at how dblaikie and echristo would have
> occasionally boisterous debates on-list when they essentially (possibly
> literally) sat next to each other.
>
>
>
> When my team started participating upstream, we were advised to try to
> avoid getting approvals from each other, and specifically seek out
> reviewers from other orgs.  Partly this is a matter of perspective, in that
> what’s good for one org and what’s good for the upstream project may be
> fairly different, and that may be clearer to someone from a different org.
> I also think if a patch goes through an internal review step before being
> posted upstream (a process we employ a fair amount in my team) then it’s
> best if someone else does the upstream review, just to avoid the
> rubber-stamp effect.  I don’t know that it’s necessary to codify this too
> strongly, it’s more of a how-to-pick-reviewers topic than how-we-do-reviews.
>

Yeah, it helps to do as much of that upstream as possible - even if it's
between two people from the same org. It helps upstream understand what
sort of scrutiny has already been applied to the patch (& allows others to
pipe up if it might be going down a path they don't agree with/might have
suggestions about)

Also there's a certain amount of the code ownership issue - & I /think/
that's more what came up (in my mind) with some contributions in the past:
Two relative newcomers to the project approving each others patches. That
to me is usually addressed by the "don't approve somtehing you wouldn't've
committed yourself without review" - except at the code owner level where
owners/frequent contributors might be seeking feedback from each other on
design decisions with no clear hierarchy between them. But I realize that
gets a bit fuzzy/awkward.

>
>
> It would be more helpful if the Code Reviews section stated a goal (or
> short goal set) for the review.  People can have radically different ideas
> about the purpose of a code review.  I attended a software-engineering
> conference talk once where Google people stated flat out that the sole
> purpose of their internal reviews is readability.  Readability is nice, it
> fosters maintainability which is certainly a good thing; but it goes
> against my entire career’s notion that reviews primarily seek out logic
> holes and defects.
>
> --paulr
>
>
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *David
> Blaikie via llvm-dev
> *Sent:* Monday, December 2, 2019 10:56 AM
> *To:* Mehdi AMINI <joker.eph at gmail.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] [RFC] High-Level Code-Review Documentation
> Update
>
>
>
> 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/ecb202e3/attachment.html>