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

[Mahesha HS]

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