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

[Chandler Carruth]

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.