[LLVMdev] [PATCH] OpenCL support - update on keywords

[Peter Collingbourne]

On Fri, Feb 18, 2011 at 03:27:13PM -0000, Anton Lokhmotov wrote:
> > -----Original Message-----
> > From: Peter Collingbourne [mailto:peter at pcc.me.uk]
> > Sent: 04 January 2011 21:42
> > To: Anton Lokhmotov
> > Cc: cfe-dev at cs.uiuc.edu
> > Subject: Re: OpenCL support
> >
> > Here are my comments on the second patch.
> > 
> > > +enum OpenCLAddressSpace {
> > > +  OPENCL_PRIVATE = 0,
> > > +  OPENCL_GLOBAL = 1,
> > > +  OPENCL_LOCAL = 2,
> > > +  OPENCL_CONSTANT = 3
> > > +};
> > 
> > If we are going to standardise these address space numbers across
> > Clang and the LLVM backends, this enum declaration should be added
> > somewhere in LLVM so that the backends have access to it.  Perhaps a
> > new header file called something like "llvm/Support/AddressSpaces.h"?
> We went for llvm/Support/OpenCL.h for now, because might need to add further
> OpenCL-specific support.  Copying to llvmdev.

I don't think we should include the word OpenCL in the name of this
enum/header.  We may very well in the future want to define address
space constants for other languages, which should not overlap with
the OpenCL constants, by extending this enum.

IMHO we shouldn't pollute the llvm namespace with this enum either.
In a similar style to the llvm/CallingConv.h header, we could
consider creating another namespace and placing the enum inside
(llvm::AddrSpace::ID perhaps?)

A few additional comments:

"private" should not be marked as a type qualifier, type specifier
qualifier or declaration specifier in languages other than OpenCL.
Otherwise we will end up accepting invalid C++ declarations such as:

private int i;

or even

int private i;

There are several 80-col violations in lib/Parse/ParseDecl.cpp
and in the CreateIntegerAttribute function.  git also picked
up some trailing whitespace errors.

To avoid the code duplication between ParseDeclarationSpecifiers,
ParseOptionalTypeSpecifier and ParseTypeQualifierListOpt you
should introduce a ParseOpenCLTypeAttributes function (e.g. see
ParseBorlandTypeAttributes).

Other than that, LGTM.

Thanks,
-- 
Peter