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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits