bader added a comment.
Hi Anastasia,
Sorry for the delay.
I have just a few comments.
Thanks,
Alexey
================
Comment at: lib/Sema/SemaChecking.cpp:95
@@ +94,3 @@
+ IllegalParams = true;
+ break;
+ }
----------------
It looks like this function will report only first invalid parameter. Can we
report all of them by moving code from lines 109-120 inside the loop?
================
Comment at: lib/Sema/SemaChecking.cpp:145
@@ +144,3 @@
+ for (unsigned i = Start; i < End; ++i)
+ if (!TheCall->getArg(i)->getType().getTypePtr()->isIntegerType())
+ return false;
----------------
If I understand it correctly the intention is to convert this integer type to
i32. Am I right?
By the spec it must be unsigned 32-bit integer, not just any integer type.
================
Comment at: lib/Sema/SemaChecking.cpp:201
@@ +200,3 @@
+/// clk_event_t *event_ret,
+/// void (^block)(void*, ...),
+/// uint size0, ...)
----------------
block arguments must be pointers to the local memory (the same as previous
declaration).
================
Comment at: lib/Sema/SemaChecking.cpp:249
@@ +248,3 @@
+ } else
+ return checkBlockArgs(S, Arg3);
+ } else if (NumArgs >= 5) {
----------------
Could you clarify that code path?
My understanding is that if NumArgs ==4, clang should except only block with
empty parameter list. Am I right?
/// int enqueue_kernel(queue_t queue,
/// kernel_enqueue_flags_t flags,
/// const ndrange_t ndrange,
/// void (^block)(void))
================
Comment at: lib/Sema/SemaChecking.cpp:250
@@ +249,3 @@
+ return checkBlockArgs(S, Arg3);
+ } else if (NumArgs >= 5) {
+ // we can have block + varargs.
----------------
This check is redundant.
It's known to be always true at this point.
================
Comment at: lib/Sema/SemaChecking.cpp:297
@@ +296,3 @@
+ return false;
+ else if (NumArgs >= 7) // check if varargs are correct types.
+ return checkEnqueueVariadicArgs(S, TheCall, Arg6, 7);
----------------
This check is redundant. This condition is known to be true at line 250.
================
Comment at: lib/Sema/SemaChecking.cpp:302
@@ +301,3 @@
+
+ // None of the specific case has been detected, give generic error
+ S.Diag(TheCall->getLocStart(),
----------------
Shouldn't by default we return false and report an error only if checks above
find inconsistency?
I expect code to be much simpler with this approach.
http://reviews.llvm.org/D20249
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
