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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.

================
Comment at: include/clang/Basic/Attr.td:671
@@ -670,3 +670,3 @@
                                             Keyword<"write_only">]>];
   let Documentation = [Undocumented];
 }
----------------
Please, if you are updating this attribute, document it.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5050
@@ +5049,3 @@
+
+  // Check if there is only one access qualifier
+  if (D->hasAttr<OpenCLAccessAttr>()) {
----------------
Missing punctuation. ;-)

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5052
@@ +5051,3 @@
+  if (D->hasAttr<OpenCLAccessAttr>()) {
+    S.Diag(D->getLocation(), diag::err_opencl_multiple_access_qualifiers)
+        << D->getSourceRange();
----------------
This should be pointing to the attribute, not the declaration, shouldn't it?

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5063
@@ +5062,3 @@
+  // qualifier is a compilation error.
+  if (const ParmVarDecl *PDecl = llvm::dyn_cast<ParmVarDecl>(D)) {
+    const Type *DeclTy = PDecl->getType().getCanonicalType().getTypePtr();
----------------
Drop the llvm namespace specifier.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5067
@@ +5066,3 @@
+      if (S.getLangOpts().OpenCLVersion < 200 || DeclTy->isPipeType()) {
+        S.Diag(D->getLocation(), diag::err_opencl_invalid_read_write)
+            << PDecl->getType() << DeclTy->isImageType() << D->getSourceRange();
----------------
This seems like it should also be pointing to the attribute instead of the declaration (I assume the declaration would be fine were it not for the presence of the attribute).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:5823
@@ -5788,8 +5822,3 @@
 
-  // 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);
----------------
Anastasia wrote:
> . missing
Missing punctuation.

Of bigger concern -- this code is highly suspect -- this is called for every declaration in the source code. Why is this complexity needed here instead of elsewhere?


http://reviews.llvm.org/D16040