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

[Mahesha HS]

Hi Matthieu,

Thanks a lot. It was a very useful information about Clang/LLVM check-in policy.

--
mahesha


On Sat, Oct 27, 2012 at 4:51 PM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
> 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
>
>



-- 
mahesha