[PATCH] D20948: [OpenCL] Fix access qualifiers handling for typedefs

[Anastasia Stulova via cfe-commits]

Anastasia added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7882
@@ -7881,1 +7881,3 @@
   "multiple access qualifiers">;
+def warn_default_access_qualifier : Warning<
+  "missing access qualifier: assuming read_only">,
----------------
I am not sure why to give a warning about deduced access qualifier? It seems perfectly ok according to the spec to default to read_only if not specified.

================
Comment at: lib/Sema/SemaType.cpp:6480
@@ -6474,2 +6479,3 @@
                                    Sema &S) {
   // OpenCL v2.0 s6.6 - Access qualifier can used only for image and pipe type.
+  if (!(CurType->isImageType() || CurType->isPipeType()) ||
----------------
Not related to your change but could you fix this typo please:
 can used -> can be used

================
Comment at: test/SemaOpenCL/images-typedef.cl:7
@@ +6,3 @@
+typedef read_only image1d_t img1d_ro;
+typedef read_write image1d_t img1d_rw;
+
----------------
Should this only be allowed for OpenCL2.0?

================
Comment at: test/SemaOpenCL/images-typedef.cl:14
@@ +13,3 @@
+void myWrite(write_only image1d_t); // expected-note {{passing argument to parameter here}} expected-note {{passing argument to parameter here}}
+void myRead (read_only image1d_t); // expected-note {{passing argument to parameter here}}
+
----------------
myRead ( -> myRead(

================
Comment at: test/SemaOpenCL/images-typedef.cl:39
@@ +38,3 @@
+
+kernel void k7 (write_only img1d_ro_default img) { // expected-error {{access qualifier can only be used for pipe and image type}}
+}
----------------
I feel this error might be a bit confusing, because the typedef is to an image at the end.

Did we have an error here before?

================
Comment at: test/SemaOpenCL/images-typedef.cl:42
@@ +41,3 @@
+
+kernel void k8 (read_only int img) { // expected-error {{access qualifier can only be used for pipe and image type}}
+}
----------------
I think this is already being tested in invalid-access-qualifier.cl.

Also could we combine with that test file into one?


http://reviews.llvm.org/D20948