----- Original Message ----- > From: "Andrey Bokhanko" <[email protected]> > To: "C. Bergström" <[email protected]> > Cc: "Alexey Bataev" <[email protected]>, > [email protected], "llvm cfe" > <[email protected]> > 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 > > > Andrey > > > > > > On Fri, May 23, 2014 at 10:00 AM, "C. Bergström" < > [email protected] > wrote: > > > 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" < > [email protected] <mailto: cbergstrom@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) > > Thanks > > ______________________________ _________________ > Openmp-dev mailing list > [email protected]. illinois.edu > http://lists.cs.uiuc.edu/ mailman/listinfo/openmp-dev > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
