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

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

Tobi

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

On Sat, Oct 13, 2012 at 2:33 AM, Mahesha HS <mahesha.llvm at gmail.com>
wrote:
> 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?
Yes, please do.

-Eli