[OPENMP] cbergstrom as OMP patch reviewer RFC

I'm changing the subject to avoid hijacking the original thread

+1

Also, Chandler, is it safe to assume that if some time (a week? two weeks?) passed and no-one but Chris (or someone else knowledgeable with OpenMP but not clang code owner) reviewed a patch, the very fact of no review [from clang code owners] suggests that the patch is not disruptive enough / too OpenMP-specific for clang code owners to step in and do code review personally? Especially given that said “clang code owners” are extremely few in numbers – like three people on Earth, if I remember correctly?

Granted, there are areas where a review from code owner is a must. But in my opinion, such areas are quite limited in case of OpenMP patches – basically, only when common infrastructure / code is changed. Agree?

Andrey

Hi Andrey,

Thanks for the feedback. I'm trying to keep this as simple as possible and only determine if my review is sufficient for the OMP stuff. Having a grace period (a few days or even a week) is a courtesy which I encourage, but as a policy I don't think it's required.

Unrelated to this specific patch, however:

    LGTM - maybe give it 1-2 more days for others to have a chance to
    review and if no objections push - Thanks

Please don't mark patches as looking good and encouraging the
contributors to commit for areas of the code and/or project you have no
familiarity with. I understand that you're quite familiar with OpenMP, but
as you aren't a regular Clang contributor (very few patches submitted, none
I could find committed directly) and aren't one of the maintainers or code
owners of Sema, it seems better to leave the final review to others.

I'm changing the subject to avoid hijacking the original thread
----------
Chandler has raised concerns I'm unqualified to review any of the OpenMP
work being submitted by Intel. I respect his opinion, but at the same time
I'd like to continue reviewing these patches where I have the time. I
obviously can't do this unless others feel I am qualified though.

I don't think that is a fair characterization of what I wrote.

First, I'm not objecting to review. Review is great, and more review from
more places is great.

I am saying that I don't think it is appropriate for someone who isn't a
regular contributor to Clang and familiar with the specific parts of Clang
(in the open, internal branches, etc. don't really count for open source
work) to sign off on a patch being committed to Clang.

To further clarify - in some cases I'm acting as a proxy to other
engineers on our team.

I don't think it is reasonable for you or anyone else to act as some form
of official proxy for the actual engineers. In the open source community,
we *need* to interact with the actual engineers directly in order to build
trust and working relationships with them. I don't see any evidence of that
being a workable strategy in LLVM certainly. Even when folks are
contributing patches originally authored by others, they both acknowledge
the authors of the work (where possible) and take full responsibility for
it going forward.

(Most of these engineers have many years of experience working with clang).

In the open source community? While I respect that many folks outside of
the open source community have expertise here, I don't think it is really
useful to insist that the open source community take that on faith with no
evidence. I also think it is specifically useful for the open source
community to encourage and incentivize people to contribute to the upstream
project.

I realize bandwidth for the clang developers to review OMP is limited - my

goal is to help provide a 2nd set of eyes on low risk contributions in
order to help get great OMP support in clang.

A second set of eyes seems awesome. But we still need someone who works
with Clang in the open source project to review the patches and OK them for
submission.

2nd - in the event my review isn't perfect and there's some intricate

detail I missed - in general the Intel team is extremely responsive and a
minor follow-up commit would likely fix any nit. (With the downside of some
commit noise, but in general I don't think every llvm/clang commit is
perfect out of the box - I can't speak for the community, but it seems
reasonable to me)

I don't think this is either a concern or relevant to the issue.

From: "C. Bergström" <cbergstrom@pathscale.com>
To: "Chandler Carruth" <chandlerc@google.com>
Cc: "Alexey Bataev" <a.bataev@hotmail.com>, openmp-dev@dcs-maillist2.engr.illinois.edu, "llvm cfe"
<cfe-commits@cs.uiuc.edu>
Sent: Friday, May 23, 2014 1:00:27 AM
Subject: [Openmp-dev] [OPENMP] cbergstrom as OMP patch reviewer RFC

> Unrelated to this specific patch, however:
>
>
> LGTM - maybe give it 1-2 more days for others to have a chance
> to
> review and if no objections push - Thanks
>
>
> Please don't mark patches as looking good and encouraging the
> contributors to commit for areas of the code and/or project you
> have
> no familiarity with. I understand that you're quite familiar with
> OpenMP, but as you aren't a regular Clang contributor (very few
> patches submitted, none I could find committed directly) and aren't
> one of the maintainers or code owners of Sema, it seems better to
> leave the final review to others.
I'm changing the subject to avoid hijacking the original thread
----------
Chandler has raised concerns I'm unqualified to review any of the
OpenMP
work being submitted by Intel. I respect his opinion, but at the same
time I'd like to continue reviewing these patches where I have the
time.
I obviously can't do this unless others feel I am qualified though.

To further clarify - in some cases I'm acting as a proxy to other
engineers on our team. (Most of these engineers have many years of
experience working with clang). I realize bandwidth for the clang
developers to review OMP is limited - my goal is to help provide a
2nd
set of eyes on low risk contributions in order to help get great OMP
support in clang. You'll see based on history I don't review random
things.

2nd - in the event my review isn't perfect and there's some intricate
detail I missed - in general the Intel team is extremely responsive
and
a minor follow-up commit would likely fix any nit. (With the downside
of
some commit noise, but in general I don't think every llvm/clang
commit
is perfect out of the box - I can't speak for the community, but it
seems reasonable to me)

If anyone wants to +1 me as a reviewer for the OMP work please speak
up.
(Otherwise I won't be able to provide further help in this area)

Please do review; no one was implying that you should not review. Let me clarify:

> LGTM

No one was objecting to this.

- maybe give it 1-2 more days for others to have a chance
> to
> review and if no objections push - Thanks

He was objecting to this: this is not how we do things. Either you're qualified to give a final review or you're not, but we don't do fixed time periods for objections. If you feel there is someone else in particular who should review a particular patch, then please name that person, cc him or her, and wait until either that person reviews or the relevant code owner intervenes. Otherwise, just remind the author that your review is not final and he or she should await a more-authoritative review. Just having review activity on a particular patch is often helpful in moving things along, and the more eyes the better.

In short, please don't take this the wrong way. We value your feedback, but we also have a need to maintain our review procedures.

Thanks again,
Hal

Also, Chandler, is it safe to assume that if some time (a week? two
weeks?) passed and no-one but Chris (or someone else knowledgeable with
OpenMP but not clang code owner) reviewed a patch, the very fact of no
review [from clang code owners] suggests that the patch is not disruptive
enough / too OpenMP-specific for clang code owners to step in and do code
review personally? Especially given that said "clang code owners" are
extremely few in numbers -- like three people on Earth, if I remember
correctly?

http://llvm.org/docs/DeveloperPolicy.html#code-reviews lays out pretty
clearly that no, it isn't OK?

Granted, there are areas where a review from code owner is a *must*. But in

my opinion, such areas are quite limited in case of OpenMP patches --
basically, only when common infrastructure / code is changed. Agree?

I think that the code owners will let you know when you have patches that
could have gone in with post-commit review. Certainly, in areas where I am
a frequent reviewer I always make sure to note in reviews when I think the
patch satisfied the "obviousness" bar or was sufficiently isolated to a
part of the code maintained by the person already.

I cannot possibly say whether or not this is the case -- I don't actually
know these parts of Clang well enough. If you get Doug, or John, or Richard
to say so, you're good-to-go. I suspect they would do so in the code review
itself.

Ok so I'm not qualified and won't be commenting on future patches. Thanks for the clarification

From: "Andrey Bokhanko" <andreybokhanko@gmail.com>
To: "C. Bergström" <cbergstrom@pathscale.com>
Cc: "Alexey Bataev" <a.bataev@hotmail.com>, openmp-dev@dcs-maillist2.engr.illinois.edu, "llvm cfe"
<cfe-commits@cs.uiuc.edu>
Sent: Friday, May 23, 2014 1:36:54 AM
Subject: Re: [Openmp-dev] [OPENMP] cbergstrom as OMP patch reviewer RFC

+1

Also, Chandler, is it safe to assume that if some time (a week? two
weeks?) passed and no-one but Chris (or someone else knowledgeable
with OpenMP but not clang code owner) reviewed a patch, the very
fact of no review [from clang code owners] suggests that the patch
is not disruptive enough / too OpenMP-specific for clang code owners
to step in and do code review personally? Especially given that said
"clang code owners" are extremely few in numbers -- like three
people on Earth, if I remember correctly?

FWIW, the a code owner (Richard) did review this patch today. Regardless, the review does not need to be from the code owner specifically, but some established contributor who is knowledgeable in that part of the code (or, to put it another way, who is comfortable taking responsibility for the commit).

Granted, there are areas where a review from code owner is a *must*.
But in my opinion, such areas are quite limited in case of OpenMP
patches -- basically, only when common infrastructure / code is
changed. Agree?

No, we need a pre-commit review for any patches with novel elements. We all, collectively, maintain all of the code.

-Hal

Chris,

Not sure my opinion weights much, but I, personally, do consider you to be qualified and we (speaking on behalf of Intel team working on OpenMP) do appreciate your code reviews – especially given how few members of the community have time / inclination to review OpenMP patches. Please keep the reviews coming!

As for what is necessary for actually letting a patch to be committed, I let more established members of clang / llvm community to decide – though in my (again, probably not very important) opinion the situation is quickly coming to a deadlock. Which is quite sad. :frowning:

Andrey