pxli168 added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:593
@@ -592,2 +592,3 @@
+def err_no_declarators : Error<"declaration does not declare anything">;
def ext_typedef_without_a_name : ExtWarn<"typedef requires a name">,
InGroup<MissingDeclarations>;
----------------
Anastasia wrote:
> I just can't understand the intention here. Could you give any code example
> or reference to spec?
I will try,
================
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">;
----------------
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.
================
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">;
----------------
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.
================
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)
----------------
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.
================
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())
----------------
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.
================
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()))
----------------
Anastasia wrote:
> Could you remove this comment?
OK. but the "|" may seems strange. I just want to explain it.
================
Comment at: lib/Sema/SemaExpr.cpp:7550
@@ -7527,3 +7549,3 @@
LHSType, RHSVecType->getElementType(),
- RHSType))
+ RHSType, Loc))
return RHSType;
----------------
Anastasia wrote:
> I am not clear about the purpose of this change.
That seems some dead experiment. ):
================
Comment at: lib/Sema/SemaExpr.cpp:10115
@@ +10114,3 @@
+ // Block.
+ if (S.getLangOpts().OpenCL && S.getLangOpts().OpenCLVersion >= 200 &&
+ Result->isBlockPointerType()) {
----------------
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.
================
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
----------------
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.
================
Comment at: lib/Sema/SemaInit.cpp:6155
@@ +6154,3 @@
+ TyQualifiers.getAddressSpace() == LangAS::opencl_global;
+ if (!HasGlobalAS && Entity.getKind() == InitializedEntity::EK_Variable &&
+ Args.size() > 0) {
----------------
Anastasia wrote:
> Should you check if it's a program scope variable? Or alternatively we could
> disallow non-pointer variables with global AS, which should have already been
> done already.
It seems so. I will have a try.
================
Comment at: test/CodeGenOpenCL/opencl_types.cl:39
@@ -38,3 @@
-
-void __attribute__((overloadable)) bad1(image1d_t *b, image2d_t *c, image2d_t
*d) {}
-// CHECK-LABEL:
@{{_Z4bad1P11ocl_image1dP11ocl_image2dS2_|"\\01\?bad1@@\$\$J0YAXPE?APAUocl_image1d@@PE?APAUocl_image2d@@1@Z"}}
----------------
Anastasia wrote:
> I see. Would it reduce test coverage? Was it potentially testing the mangling
> of image types?
>
> I think it's best to change the code instead of completely removing it. May
> be just remove * so it will be parsed. We seem not to have any test for
> mangling the images.
>
Thank you advice, if may test for the mangling.
But the name bad make me the think it is a useless test so I just remove it.
I will change them into no-pointer type.
================
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}}
+}
----------------
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.
================
Comment at: test/SemaOpenCL/invalid-decl.cl:6
@@ +5,2 @@
+ myfun(); // expected-error {{implicit declaration of function 'myfun' is
invalid in OpenCL}}
+}
----------------
Anastasia wrote:
> Yes, I think having it in parser is wrong. Not sure why it's there. You can
> move it if you wish. Otherwise, I will do afterwards.
Keep them be together, and easy to move to right place then.
http://reviews.llvm.org/D16047
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
