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

[Mahesha HS]

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:
>

Hi Chandler,

> 1) The over-use of OwningPtr seems like a serious code smell. I would
> want to understand why these are pointers at all here.

In general, any pragma support in Clang requires it to be first
registered with Preprocessor. The way to register it is by deriving a
new class from pragma base class interface - "class Pragma", and then
by adding an instance of the derived class to Preprocessor's pragma
handler object. Since OpenMP has around *fifteen* different pragma
directives, we need a separate instance for each of them. So, there
seems to be a over-use of OwningPtr.

However, as I am new to Clang code base, my current usual practice is
to look into exiting relevant code base and try to follow the same
approach when it comes to allocating new objects, conservative
implementation, etc. I looked into the Parser class, where already
OwningPtr is used liberally to some extent to support few pragmas. So,
 I thought of following the same approach.

However, your comment is well taken. I will see if I can do it more
efficient way. I also welcome your suggestions in this regard.

> 2) There are copious "doxygen" formatted comments that don't use a
> doxygen comment prefix of '///', instead using the normal '//'.

I will be very careful with these violations.

> 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.

If possible, please give me some hints so that it helps me to create
more robust test cases. This is very crucial at this point since I am
still new to Clang environment.

> 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.

By default, OpenMP feature is *off* in all most all compilers. Only
when the user passes -fopenmp, it will be turned *on*. So, I did not
think about  -fno-openmp. However, I will  think about it, and support
it too, if we decide that it will be useful.

-- 
mahesha