Anastasia added inline comments. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7670 @@ +7669,3 @@ + "%0 can only be used as a function parameter">; +def err_opencl_atomic_init_addressspace : Error< + "initialization of atomic variables is restricted to variables in global address space in opencl">; ---------------- pxli168 wrote: > Anastasia wrote: > > Could you do something like: > > > > def err_atomic_init_constant : Error< > > "atomic variable can only be %select{assigned|initialised}0 to a > > compile time constant" > > " in the declaration statement in the program scope">; > > > Seems good. But that error message was from spec, this will change it. You can pick the wording you like. I personally find this one better!
================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7674 @@ +7673,3 @@ + "invalid block prototype, variadic arguments are not allowed in opencl">; +def err_opencl_invalid_block_array : Error< + "array of block is invalid in OpenCL">; ---------------- pxli168 wrote: > Anastasia wrote: > > Could we combine err_opencl_invalid_block_array and > > err_opencl_pointer_to_image saying something like: > > > > "Declaring a %select{pointer|array}0 of type %1 is not allowed in OpenCL" > OK, it will save few lines. And more importantly won't populate diagnostics. ================ Comment at: lib/Sema/SemaDecl.cpp:5724 @@ +5723,3 @@ + R->isPipeType()) { + Diag(D.getIdentifierLoc(), + diag::err_opencl_type_can_only_be_used_as_function_parameter) ---------------- pxli168 wrote: > Anastasia wrote: > > Some of them have to be. For example for C types that we use differently in > > CL. But for CL only types do we need to check additionally that it's CL? Do > > we not know it already? > Yes, but it seems all old code write in that way. I just follow the style. Ok, I think an improvement to the old code is always welcome! ================ Comment at: lib/Sema/SemaExpr.cpp:6299 @@ -6286,3 +6298,3 @@ // Now check the two expressions. if (LHS.get()->getType()->isVectorType() || RHS.get()->getType()->isVectorType()) ---------------- pxli168 wrote: > Anastasia wrote: > > I am not sure what the question is? > > > > I think using block in a condition should be disallowed. Could you add this > > to testing as well? > I don't see any thing talk about the block and condition. Spec only tells > about block and selection. Ok, in that case there is nothing left to do here I guess. ================ Comment at: lib/Sema/SemaExpr.cpp:6316 @@ +6315,3 @@ + if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion >= 200) { + // should output error for both LHS and RHS, use | instead || + if (checkBlockType(*this, LHS.get()) | checkBlockType(*this, RHS.get())) ---------------- pxli168 wrote: > Anastasia wrote: > > Could you remove this comment? > OK. but the "|" may seems strange. I just want to explain it. Doesn't seem strange to me. ================ Comment at: lib/Sema/SemaExpr.cpp:10115 @@ +10114,3 @@ + // Block. + if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 && + Result->isBlockPointerType()) { ---------------- pxli168 wrote: > Anastasia wrote: > > The code above seems to do similar. Could we combine into one > > function/diagnostic? > Yes, they are. But they live in different function test for operand "&" and > "*" and maybe it is hard to merge them together. I have uploaded the full > diff as you asked and uou can expand the code to see they are in two function > check for the two unary operators. Yes, it's not a problem that they are in different functions. Could this code be encapsulated in a function, let's say checkBlockType(...) and be called from multiple places? ================ Comment at: lib/Sema/SemaInit.cpp:6139 @@ +6138,3 @@ + // OpenCL v2.0 s6.13.11.1 - The ATOMIC_VAR_INIT macro expands to a token + // sequence suitable for initializing an atomic object of a type that is + // initialization-compatible with value. An atomic object with automatic ---------------- pxli168 wrote: > Anastasia wrote: > > I don't think we need to copy the spec text here. I would like to see some > > explanation of the code actually. > > > > Could you write something like: "Non-program scope atomic variables can't > > be initialized." > But the example said there can be program scope atomic that only in global > address space. > > > This macro can only be used to initialize atomic objects that are declared > > in program scope in the global address space > > The quote from spec said so. Yes, so copying spec into code is not the goal. Let's try to make it shorter and still have the same meaning! ================ Comment at: test/Parser/opencl-atomics-cl20.cl:71 @@ +70,3 @@ +void atomic_init_test() { + atomic_int guide = ATOMIC_VAR_INIT(42); // expected-error {{initialization of atomic variables is restricted to variables in global address space in opencl}} +} ---------------- pxli168 wrote: > Anastasia wrote: > > Why using a macro here? It seems to complicate the test without adding any > > functionality. > Because the the init type maybe different and then have another error. > > I will have a test for this semacheck as I said above. The macro is removed by the preprocessor and it's empty here anyways. You are not adding a builtin macro ATOMIC_VAR_INIT to Clang. This test can look a lot cleaner (without doing any macro expansion): atomic_int guide = 42; http://reviews.llvm.org/D16047 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits