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

[Eli Friedman]

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