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 <[email protected]> wrote: > On Tue, Oct 16, 2012 at 4:11 AM, Mahesha HS <[email protected]> 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
fopenmp_option_support.patch
Description: Binary data
omp_pragma_registration.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
