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

[Mahesha HS]

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 <gribozavr at gmail.com> wrote:
> Hi Mahesha,
>
> On Wed, Oct 24, 2012 at 3:05 PM, Mahesha HS <mahesha.llvm at gmail.com> 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 <gribozavr at gmail.com>*/



-- 
mahesha