[PATCH] D71005: [AST] Enable expression of OpenCL language address spaces an attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1123
 def OpenCLPrivateAddressSpace : TypeAttr {
-  let Spellings = [Keyword<"__private">, Keyword<"private">];
+  let Spellings = [Keyword<"__private">, Keyword<"private">, Clang<"opencl_private">];
   let Documentation = [OpenCLAddressSpacePrivateDocs];
----------------
bader wrote:
> aaron.ballman wrote:
> > This looks like we're adding two levels of namespace -- is there a reason this should be `clang::opencl_private` as opposed to `opencl::private`? Is there something clang-specific to these?
> > is there a reason this should be clang::opencl_private as opposed to opencl::private?
> 
> I'm okay with [[opencl::private]] as well. I have only one problem - currently OpenCL address spaces are exposed as keywords and using them in C++ breaks valid C++ code.
> 
> > Is there something clang-specific to these?
> 
> I guess no (except that it's implemented in only Clang :-) ).
> 
> @aaron.ballman, I already committed this version, sorry about that. I'll open another review with attribute renaming.
>> is there a reason this should be clang::opencl_private as opposed to opencl::private?
> I'm okay with [[opencl::private]] as well. I have only one problem - currently OpenCL address spaces are exposed as keywords and using them in C++ breaks valid C++ code.

I'm not certain who controls the OpenCL spec, but this seems like it should be a decision that comes from there. Or is this functionality outside of the OpenCL spec?

In attributes, when an identifier can be interpreted as a keyword it is required to be interpreted as an identifier. We have the same issue with `[[gnu::const]]`. See http://eel.is/c++draft/dcl.attr.grammar#4.sentence-5

>> Is there something clang-specific to these?
> I guess no (except that it's implemented in only Clang :-) ).

I mostly mean: will other implementations of OpenCL want to have the same functionality? Or is this something we expect other OpenCL implementations to largely ignore?

> @aaron.ballman, I already committed this version, sorry about that. I'll open another review with attribute renaming.

The turnaround time between opening the review and closing it was pretty quick, but you had a reasonable LG so it's no big deal. We can continue chatting about it on this review until we decide that changes really are needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71005/new/

https://reviews.llvm.org/D71005