[PATCH] D16686: [OpenCL] Generate metadata for opencl_unroll_hint attribute

[Xiuli PAN via cfe-commits]

pxli168 added inline comments.

================
Comment at: lib/Sema/SemaStmtAttr.cpp:208
@@ +207,3 @@
+                                    SourceRange Range) {
+  assert(A.getKind() == AttributeList::AT_OpenCLUnrollHint);
+
----------------
This is also not necessary because this function can be called only if it is an OpenCLUnrollHint

================
Comment at: lib/Sema/SemaStmtAttr.cpp:210
@@ +209,3 @@
+
+  // opencl_unroll_hint can have 0 arguments (compiler determines unrolling
+  // factor) or 1 argument (the unroll factor provided by the user).
----------------
You may put the reference at the begin and make a summary after that, I don't think see ... for details is need.
Just like what other OpenCL references. 

================
Comment at: lib/Sema/SemaStmtAttr.cpp:217
@@ +216,3 @@
+  if (NumArgs > 1) {
+    S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << 1;
+    return 0;
----------------
> def err_attribute_too_many_arguments : Error<"%0 attribute takes no more than %1 argument%s1">;

you should give this two arguments, one is name and another is the number you expected.




================
Comment at: lib/Sema/SemaStmtAttr.cpp:218
@@ +217,3 @@
+    S.Diag(A.getLoc(), diag::err_attribute_too_many_arguments) << 1;
+    return 0;
+  }
----------------
please use nullptr
should not use 0 in C++11.

================
Comment at: lib/Sema/SemaStmtAttr.cpp:225
@@ +224,3 @@
+    Expr *E = A.getArgAsExpr(0);
+    assert(E != nullptr && "Invalid opencl_unroll_hint argument");
+    llvm::APSInt ArgVal(32);
----------------
Is this necessary as you have checked there is only one arg in this attr? 

================
Comment at: lib/Sema/SemaStmtAttr.cpp:230
@@ +229,3 @@
+      S.Diag(A.getLoc(), diag::err_attribute_argument_type)
+          << A.getName()->getName() << AANT_ArgumentIntegerConstant
+          << E->getSourceRange();
----------------
Why there is two getName?

================
Comment at: test/SemaOpenCL/unroll-hint.cl:17
@@ +16,3 @@
+kernel void E() {
+  __attribute__((opencl_unroll_hint(2,4))) // expected-error {{1 attribute takes no more than 1 argument}}
+  for(int i=0; i<100; i++);
----------------
I think

> expected-error {{**1 attribute **takes no more than 1 argument}}

should be opencl_unroll_hint


http://reviews.llvm.org/D16686