Sorry, in my previous mail, I had missed to attach changes to "clang/include/clang/Basic/TokenKinds.def" in the patch 2. Please refer to the patch (2) attached in *this* mail, instead of the one sent in the previous mail. Patch 1 is fine.
-- mahesha On Thu, Oct 18, 2012 at 5:02 PM, Mahesha HS <[email protected]> wrote: > 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 -- mahesha
omp_pragma_registration.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
