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

Eli Friedman eli.friedman at gmail.com
Sat Oct 13 03:09:34 PDT 2012

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.


More information about the llvm-dev mailing list