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

[Mahesha HS]

Hi Eli,

Thanks for all your comments.

I have taken care of all your review comments. Yes, after I gone
through your review
comments, I also came to the conclusion that the addition of a new
class for OpenMP
pragma handling (class PragmaOmpHandler) is not necessarily required.
However, initially, when  started, I my-self had an opinion that this
class may required.
Now, I removed this class, and moved the omp pragma registration directly into
Preprocessor class.

I have attached following two patches along with this mail. The
patches also contains
 relevant *test cases*.

patch 1:  fopenmp_option_support.patch
patch 2:  omp_pragma_registration.patch

In patch 1, I have taken care of all your earlier review comments
related to -fopenmp option
support. Patch 2 contains the implementation for the omp pragma
registration with
Preprocessor.

Following files are changed respectively.

patch 1:
------------
#.  clang/include/clang/Driver/Options.td
#.  clang/include/clang/Basic/LangOptions.def
#.  clang/lib/Driver/Tools.cpp
#.  clang/lib/Frontend/CompilerInvocation.cpp
#.  clang/test/Driver/clang_fopenmp_opt.c

patch 2:
------------
#.  clang/include/clang/Lex/Preprocessor.h
#.  clang/lib/Lex/Preprocessor.cpp
#.  clang/lib/Lex/Pragma.cpp
#.  clang/test/Preprocessor/pragma_omp_ignored_warning.c

--
mahesha


On Wed, Oct 17, 2012 at 7:00 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Tue, Oct 16, 2012 at 4:11 AM, Mahesha HS <mahesha.llvm at gmail.com> wrote:
>> Hi Eli,
>>
>> Attached is the next patch in the line. This patch
>> (class_pragma_omp_handler_support.patch) contains the implementation
>> of the class "class PragmaOmpHandler".  I also attached the test case
>> (openmp_syntax_test.c).  This test case is actually to test the
>> syntactically legal simple OpenMP constructs. However, we can *really*
>> test it only after submitting the next two patches - one related to
>> PragmaOmpHandler object construction and the another related to OpenMP
>> parsing.
>>
>> Please, let me know, if you need to know any additional information.
>>
>> If you think that some other mechanism is required to speed-up the
>> review process, I will really welcome it.
>
> At this point, it's just a matter of finding time to review it.
>
> +  // \Brief: Holds all the OpenMP clause name strings.
> +  // \Brief: These strings are referenced while parsing OpenMP clauses.
> +  // Note that we won't introduce new *tokens* for openMP clause names
> +  // as these will get conflict with *identifier* token, and is very tricky
> +  // to handle. Instead, we reference below strings to recognize the OpenMP
> +  // clause names.
> +  StringRef* Clause[END_CLAUSE];
>
> This isn't correct documentation comment syntax; clang trunk has a
> warning -Wdocumentation to catch this.
>
> It's almost never appropriate to construct a StringRef*; a StringRef
> is already essentially a pointer.
>
> Is this array even necessary?  It's not clear what you're using it
> for.  If you want to convert from the enum to a string, just implement
> a method like "static StringRef StringForDefaultKind(ClauseKind
> Kind);" or something like that.
>
> +  // Note: These enum values *must* be in consistant in *size* and *order* with
> +  // that of enum values defined in the AST node class "OmpClause".
>
> If you want to share an enum, put it into a header in
> include/clang/Basic/.  (A new header if no existing one is
> appropriate.)  Having the same enum in multiple places is asking for
> trouble.
>
> This patch has a lot of copy-paste code; can you write a single
> PragmaHandler subclass which is parameterized based on the pragma in
> question?  (Or maybe a few, since it looks like some of them need
> special handling which is not yet implemented?)
>
> It looks like the only members of PragmaOmpHandler which actually need
> to be in a header are PragmaOmpUnknownWarning, initialize method, and
> the enums; please move PragmaOmpUnknownWarning and initialize onto the
> Preprocessor class, the enums into a new header in
> include/clang/Basic/, and everything else into the .cpp file, and
> remove include/clang/Lex/PragmaOmpHandler.h.
>
> -Eli



-- 
mahesha
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fopenmp_option_support.patch
Type: application/octet-stream
Size: 3038 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/45f3d5b2/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: omp_pragma_registration.patch
Type: application/octet-stream
Size: 9631 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121018/45f3d5b2/attachment-0001.obj>