[Openmp-dev] [OPENMP] cbergstrom as OMP patch reviewer RFC

[Hal Finkel]

----- Original Message -----
> From: "C. Bergström" <cbergstrom at pathscale.com>
> To: "Chandler Carruth" <chandlerc at google.com>
> Cc: "Alexey Bataev" <a.bataev at hotmail.com>, openmp-dev at dcs-maillist2.engr.illinois.edu, "llvm cfe"
> <cfe-commits at cs.uiuc.edu>
> Sent: Friday, May 23, 2014 1:00:27 AM
> Subject: [Openmp-dev] [OPENMP] cbergstrom as OMP patch reviewer RFC
> 
> On 05/23/14 11:55 AM, Chandler Carruth wrote:
> > Unrelated to this specific patch, however:
> >
> > On Thu, May 15, 2014 at 4:17 AM, "C. Bergström"
> > <cbergstrom at pathscale.com <mailto:cbergstrom at pathscale.com>> wrote:
> >
> >     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

> 
> Thanks
> 
> _______________________________________________
> Openmp-dev mailing list
> Openmp-dev at dcs-maillist2.engr.illinois.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/openmp-dev
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory