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

[Mahesha HS]

On Sat, Oct 13, 2012 at 1:31 PM, Tobias Grosser <tobias at grosser.es> wrote:
> On 10/13/2012 04:38 AM, Mahesha HS wrote:
>>
>> On Sat, Oct 13, 2012 at 5:14 AM, Eli Friedman <eli.friedman at gmail.com>
>> wrote:
>>>
>>> On Wed, Oct 10, 2012 at 6:15 AM, Mahesha HS <mahesha.llvm at gmail.com>
>>> wrote:
>>>>
>>>> Hi Eli and Others
>>>>
>>>> In response to your feedback, I have taken care of all your review
>>>> comments
>>>> - I removed clangOMP.a
>>>> and moved the implementation of "class OmpPragmaHandler" to
>>>> "clangLex.a".
>>>>
>>>> The attached zipped file - namely - 'OpenMP_support_in_Clang.tar.gz'
>>>> contains all the implemented
>>>> "patches" along with *newly* added source files.
>>>>
>>>> Another attached text file - namely -
>>>> 'OpenMP_support_in_Clang_Read_Me.txt'
>>>> briefly describes the
>>>> implementation.
>>>>
>>>> You can also refer to the initial proposal document - namely -
>>>> 'OpenMP_Support_in_Clang.pdf'.
>>>>
>>>> Looking forward for your review comments.
>>>
>>>
>>> Can you please split this patch somehow?  This is way too big to
>>> easily review.  I'd like to see separate patches for the command-line
>>> option, the pragma handler itself, parsing each pragma, each new AST
>>> node, and semantic analysis for each kind of pragma, with testcases
>>> for every patch.  And the more you can split them, the better to get
>>> through reviews faster.
>>>
>>> Please include new files in patches using "svn add"; don't send new
>>> files separately.
>>>
>>> Please look over patches before submitting to make sure you don't
>>> include unnecessary changes to whitespace etc.
>>
>>
>> Thanks for the reply. Sure, I will again send updated patches which
>> take care of all the above comments.This is the first time, I am
>> sending the patch to CLANG/LLVM community in particular, and to any
>> open source community, in general. Also, I was in a hurry to send the
>> patch based on your first reply. Will be sending the updated patches
>> soon.
>
>
> Hey Mahesh,
>
> thanks for working on this. As a small hint from my side, I have the feeling
> there is currently no need to submit the entire OpenMP patch for review. You
> could e.g. start with the patch that adds the (command
> line option, but just ignores it. As a next step, you could submit a patch
> that makes  >>include "omp.h" << work (without yet supporting
> any definitions, ...)
>
> I think at the beginning it is helpful to start with smaller patches and to
> get used to the review policies. This will prepare you for the
> larger discussions with Elli and Co. :-)

Hi Tobias,

Incremental reviews (and check-ins) make sense actually.  I think, it
helps both me and Eli (and others) in several ways.

@ Eli,

Is it fine with you too?

--
mahesha

>
> Tobi
>
>
>



-- 
mahesha