On Sun, Oct 21, 2012 at 4:43 AM, Mahesha HS <[email protected]> wrote: > On Sat, Oct 20, 2012 at 4:38 PM, Mahesha HS <[email protected]> wrote: >> On Fri, Oct 19, 2012 at 4:03 AM, Eli Friedman <[email protected]> wrote: >>> On Thu, Oct 18, 2012 at 6:34 AM, Mahesha HS <[email protected]> wrote: >>>> 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. >>> >>> +//-------------------------------- >>> +// OpenMP directives >>> +//-------------------------------- >>> +// \#pragam omp parallel ... >>> +TOK(pragma_omp_parallel) >>> +// \#pragam omp for ... >>> +TOK(pragma_omp_for) >>> >>> Please use ANNOTATION(pragma_omp_parallel) etc. instead. >> >> I have taken care of it. Also, for patch 1 related test case, I >> replaced -v by -###. >> >>> >>> +/// PragmaOmpHandler - "\#pragma omp ...". >>> +template<class T, tok::TokenKind TKind> >>> +struct PragmaOmpHandler : public PragmaHandler { >>> + PragmaOmpHandler() : PragmaHandler(T::Name) {} >>> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind >>> Introducer, >>> + Token &OmpTok) { >>> + PP.HandlePragmaOmp(OmpTok, TKind); >>> + } >>> +}; >>> >>> Something like following would make your patch substantially shorter: >>> >>> struct PragmaOmpHandler : public PragmaHandler { >>> tok::TokenKind TKind; >>> PragmaOmpHandler(tok::TokenKind Kind, StringRef Name) >>> : PragmaHandler(Name), TKind(Kind) {} >>> virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind >>> Introducer, >>> Token &OmpTok) { >>> PP.HandlePragmaOmp(OmpTok, TKind); >>> } >>> }; >> >> My mis-understanding of PragmaNamespace::AddPragma() function, in >> particular, mis-understanding of *assertion* statement within this >> function mislead me. I was in an impression that, adding same class >> more than once to same pragma namespace will result in assertion >> failure. But, I had not carefully looked into it that it should be >> fine as long as the associated names are different. Anyway, I have >> taken care of this too. >> >>> >>> >>> Please add a test that the pragmas round-trip correctly through clang -E. >> >> This test case is getting an assertion failure. Looks like, all the >> #pragmas which requires parsing, has to be registered (to >> preprocessor) from libParse.a just like how #pragma unused, etc are >> handled? When -E option is added, these pragmas are handled by >> Preprocessor as unknown pragmas. >> >> Waiting for more clarification from you. > > I have proceeded further, and followed exactly the similar approach as > implemented in case of other pragmas like #pragma unused, etc. > Attached is the revised patch (omp_pragma_registration_revised.patch). > > In case of any further comments, please let me know. Otherwise, we > need to check-in both patch 1 (fopenmp_option_support.patch) and patch > 2 (this one).
You never sent the revised fopenmp_option_support.patch. Index: include/clang/Lex/Preprocessor.h =================================================================== --- include/clang/Lex/Preprocessor.h (revision 166387) +++ include/clang/Lex/Preprocessor.h (working copy) @@ -161,6 +161,13 @@ /// \brief True if we are pre-expanding macro arguments. bool InMacroArgPreExpansion; + /// \brief When an OpenMP pragma is ignored, we emit a warning message saying + /// so, but only once per translation unit irrespective of the number of + /// OpenMP pragmas appeared in the translation unit. This flag keeps track of + /// whether the unkown pragma warning message is emitted or not for the + /// current translation unit. + bool PragmaOmpUnknownWarning; Please move this into the parser, since you've moved everything else. (You might need to store a reference to the flag in PragmaOmpHandler.) Otherwise, looks fine. -Eli _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
