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

[Eli Friedman]

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