kenba/opencl3

Use `bool` instead of `cl_bool`

Closed this issue · 4 comments

vmx commented

When you search the opencl3 source code for cl_bool, you find some occurrences in the public API. I propose changing them to a Rust bool type.

cl_bool is a type alias for u32, so you need to use in 0 and 1 (or use the CL_FALSE/CL_TRUE constants), and cannot just use true or false, which I'd find more ergonomic.

I bring it up as #22 is a breaking change, so it might be the right time to do more related breaking changes like this one.

kenba commented

Yes Volker, I agree that the calls that return cl_bool in the spec should be cast to Rust bool in opencl3 to make the changes of issue #21 consistent across opencl3.

Please feel free to implement the change in a PR again. However, please use != CL_FALSE as per the comment in PR #22.

As for the change breaking the API, it's not a problem I'll simply release the change under version '0.3.0'.

vmx commented

I'm not only talking about returns, but also input values, e.g. in enqueue_read_buffer.

Finding all functions that return a cl_bool might be harder to find, as some don't return cl_bool, but a cl_uint as in some cases in cl3 there is not distinction made between those two. You would really need to search the spec.

As for the change breaking the API, it's not a problem I'll simply release the change under version '0.3.0'.

Yes, what I meant was, that I wanted to do all those changes before doing a 0.3 release and not getting a 0.3 out and then doing changes we already know that will need a 0.4.

kenba commented

On searching for cl_bool in the opencl3 code, I didn't find any more return examples, just parameters in the CommandQueue enqueue_* methods and the Sampler::create, normalize_coords parameter.

The Sampler::create has a couple of other parameters that are also OpenCL typedefs: cl_addressing_mode and cl_filter_mode.
Changing the normalize_coords parameter to a Rust bool is unlikely to improve the API of that method.

The CommandQueue enqueue_* methods are even less likely to be improved by using a rust bool.
The cl_bool controls whether it's a blocking method which is best controlled using the cl-sys constants defined as CL_BLOCKING = CL_TRUE and CL_NON_BLOCKING = CL_FALSE and shown in the updated integration_test.rs code.

As for all functions that return a cl_bool, you would have to search the spec to find any that were missed in cl3 or cl-sys.
I don't think that you need to find them, but whether you want to or not is entirely up to you.

vmx commented

Thanks for the detailed analysis. I actually sue CL_BLOCKING for CommandQueue enqueue_* already. As you know, I'd make the opencl3 API a bit higher level and in that case introducing an enum with Blocking/NonBlocking and would use that as input value for enqueue_*.

I currently don't have the need for having all functions return Rust bools, I won't do that for now, hence closing this issue.