[PATCH] D16040: [OpenCL] Refine OpenCLImageAccessAttr to OpenCLAccessAttr

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5046
@@ +5045,3 @@
+  if (D->hasAttr<OpenCLAccessAttr>()) {
+    S.Diag(D->getLocation(), diag::err_opencl_multiple_access_qualifiers)
+        << D->getSourceRange();
----------------
That's why I think it should be pointing at the attribute. The presence of the second attribute is what triggers the diagnostic, not anything about the declaration. The location should always point to "this thing is what caused the diagnostic."

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5062
@@ +5061,3 @@
+        S.Diag(Attr.getLoc(), diag::err_opencl_invalid_read_write)
+          //TODO
+            << PDecl->getType() << DeclTy->isImageType() << D->getSourceRange();
----------------
Spurious TODO?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5804-5816
@@ -5768,9 +5803,15 @@
 
-  // Walk the declarator structure, applying decl attributes that were in a type
-  // position to the decl itself.  This handles cases like:
-  //   int *__attr__(x)** D;
-  // when X is a decl attribute.
-  for (unsigned i = 0, e = PD.getNumTypeObjects(); i != e; ++i)
-    if (const AttributeList *Attrs = PD.getTypeObject(i).getAttrs())
-      ProcessDeclAttributeList(S, D, Attrs, /*IncludeCXX11Attributes=*/false);
+  // Skip pipe type, otherwise it will be processed twice with its element type
+  const ParmVarDecl *PDecl = llvm::dyn_cast<ParmVarDecl>(D);
+  if (!PDecl ||
+      !PDecl->getType().getCanonicalType().getTypePtr()->isPipeType()) {
+    // Walk the declarator structure, applying decl attributes that were in a
+    // type position to the decl itself.  This handles cases like:
+    //   int *__attr__(x)** D;
+    // when X is a decl attribute.
+    for (unsigned i = 0, e = PD.getNumTypeObjects(); i != e; ++i) {
+      if (const AttributeList *Attrs = PD.getTypeObject(i).getAttrs())
+        ProcessDeclAttributeList(S, D, Attrs, /*IncludeCXX11Attributes=*/false);
+    }
+  }
 
----------------
pxli168 wrote:
> I think this attribute one that need to be slid on the declaration. Now I just handle this in ProcessDeclAttributes, with
> 
> ```
>   const ParmVarDecl *PDecl = llvm::dyn_cast<ParmVarDecl>(D);
>   if (!PDecl ||
>       !PDecl->getType().getCanonicalType().getTypePtr()->isPipeType()) {
> ```
> And I tried to handle it in  distributeTypeAttrsFromDeclarator but it seems this attribute does not go through that path and I am still working on how to place this check.
You should look into why it does not make its way into distributeTypeAttrsFromDeclarator(); as it stands, I am uncomfortable with the current approach.


http://reviews.llvm.org/D16040