Hi Dmitri, Thanks for the review. I have taken care of all your comments. With respect to the use of std::lower_bound() instead of our own binary search implementation, looks like, it is *not* straightforward as the search algorithm actually need to operate on a map table which holds the pair <StringRef, Enum Type>. However, I will re-think about it and modify it if it is possible in some way.
I will be committing first three patches as most of the review comments are taken care for them. I will cross verify and make sure that these patches will *strictly* adhere to CLANG/LLVM coding standard before committing. -- mahesha On Sat, Oct 27, 2012 at 12:28 AM, Dmitri Gribenko <[email protected]> wrote: > Hi Mahesha, > > On Wed, Oct 24, 2012 at 3:05 PM, Mahesha HS <[email protected]> wrote: >> In revised patch 3, I removed all global variables as you suggested, >> and implemented simple *const* string tables using simple binary >> search algorithm. > > Is it possible to use std::lower_bound()? > > +//===----------------------------------------------------------------------===// > +// OpenMP - An instance of this interface is defined to initiate > OpenMP parsing, > +// and to help Parser during subsequent parsing of OpenMP constrcuts. > +//===----------------------------------------------------------------------===// > +class OpenMP { > > Please use standard Doxygen. > > + // brief The member object reference to the Preprocessor. > + Preprocessor &PP; > + > + // -brief Handler objects which handle different OpenMP pragma directive > + // statements. > > '\brief' and three slashes, please (here and below). > > + bool PragmaOmpUnknownWarning; > > This is possibly bikeshedding, but I would call that PragmaOmpUnknownWarned. > > +ANNOTATION(pragma_omp_parallel) > +ANNOTATION(pragma_omp_for) > +ANNOTATION(pragma_omp_sections) > ... > > Bikeshedding: is there some ordering criteria in this list (and other > code sequences using these constants)? Seems like it has the same > order as in the OpenMP spec. Please add comment about that so that > future maintainers will keep this order when adding support for new > directives. > > +/// \brief getScheduleKind - Helper routine to get an enum kind associated > > Please do not duplicate routine name in documentation comment. Yes, > old code is written that way, but current coding guidelines recommend > against it. > > Please do not duplicate the same comment in .h and .cpp. > > Dmitri > > -- > main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if > (j){printf("%d\n",i);}}} /*Dmitri Gribenko <[email protected]>*/ -- mahesha _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
