[cfe-commits] [LLVMdev] [cfe-dev] OpenMP support in CLANG: A proposal

[Matthieu Monrocq]

Hi Mahesha,

I understand it may be difficult to get how things are working here, so let
me explain what I have understood of it (so far). Do mind that I am mostly
a lurker here so my words may be slightly off.

There are two categories of developers working on Clang: those who are
trusted and those who are not trusted *yet*. Clang is a large body of code,
with many areas, so it will take time before a newcomer is comfortable in
coding there and understand the implications of his work, and he/she will
probably not be trusted before this happens.

In general, those who are trusted will commit and their patch will get
reviewed after the fact; unless they want to consult their peers and
voluntarily submit themselves to a pre-commit review to gather opinions
(generally on the cfe-dev list).

Those who are not trusted yet should ask for approval before committing
every time for non-trivial patches (trivial patches are for example
typo/grammaro corrections); this approval will be formal (sometimes
abbreviated to LGTM, other times more explicit).

Being new, you are not trusted yet and so will need to get approval from
the code owner of the area you are working on. This approval will only be
given after one or several reviews, please do not be afraid of them, it is
expected that it takes time to learn new coding guidelines *and* new idioms
*and* the organization and goals of each piece!

Because reviewing takes so much time, many people may participate, but only
the code owner of the area you have been working on may deliver the final
go-ahead.

Finally, the last round may not be completely "spotless", and you may get a
comment such as "with those fixed, LGTM" which indicates that the code is
good to go after the few fixes evoked in above. This should be the only
incidence where you can commit a patch without further approval.

Hope this helps.

-- Matthieu

On Sat, Oct 27, 2012 at 1:00 PM, Mahesha HS <mahesha.llvm at gmail.com> wrote:

> Hi Chandler,
>
> I reverted back all the changes. I am sorry. I was in an impression
> that code is in a good shape to commit as it had went through few
> rounds of review. Also, I  had misread the line "Code can be reviewed
> either before it is committed or after"  from
> http://llvm.org/docs/DeveloperPolicy.html
>
> Thanks for your review comments. I will go through them. And, I will
> wait till I get a formal approval.
>
> --
> mahesha
>
>
> On Sat, Oct 27, 2012 at 2:48 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
> > On Sat, Oct 27, 2012 at 2:10 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
> >> On Fri, Oct 26, 2012 at 11:14 PM, Mahesha HS <mahesha.llvm at gmail.com>
> wrote:
> >>> Hi Dmitri,
> >>>
> >>> Thanks for the review. I have taken care of all your comments. With
> >>> respect to the use of std::lower_bound() instead of our own binary
> >>> search implementation, looks like, it is *not* straightforward as the
> >>> search algorithm actually need to operate on a map table which holds
> >>> the pair <StringRef, Enum Type>. However, I will re-think about it and
> >>> modify it if it is possible in some way.
> >>>
> >>> I will be committing first three patches as most of the review
> >>> comments are taken care for them. I will cross verify and make sure
> >>> that these patches will *strictly* adhere to CLANG/LLVM coding
> >>> standard before committing.
> >>
> >> I'm really sorry, but that is *not* the policy of pre-commit review,
> >> or commit access after *approval*.
> >>
> >> Please wait until you get explicit approval, especially for such very
> >> large feature additions to Clang. Reviewers often focus on one set of
> >> issues for a particular round of review and may have further issues
> >> they need you to address before committing.
> >
> > I see you have already committed all three without waiting for
> > approval. Please don't abuse the commit privilege in this way.
> >
> > The code still has some demonstrable problems by the way, which I
> > found with only a casual glance. I would assume that these would have
> > come up in subsequent review:
> >
> > 1) The over-use of OwningPtr seems like a serious code smell. I would
> > want to understand why these are pointers at all here.
> > 2) There are copious "doxygen" formatted comments that don't use a
> > doxygen comment prefix of '///', instead using the normal '//'.
> > 3) The test cases seem quite haphazard; these are often the last
> > things to be addressed in code review, but possibly I'm just skimming
> > the patch in too little detail and much of the testing will come
> > later.
> > 4) There is no -fno-openmp to complement -fopenmp. Having this ability
> > to turn off features by appending flags is something we strive for in
> > the driver.
>
>
>
> --
> mahesha
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121027/91642fd4/attachment.html>