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

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.

-Eli

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.

-mahesha

>
> -Eli


-- 
mahesha

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