Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use bool instead of cl_bool #24

Closed
vmx opened this issue Jul 8, 2021 · 4 comments
Closed

Use bool instead of cl_bool #24

vmx opened this issue Jul 8, 2021 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@vmx
Copy link
Contributor

vmx commented Jul 8, 2021

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
Copy link
Owner

kenba commented Jul 9, 2021

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'.

@kenba kenba assigned vmx Jul 9, 2021
@kenba kenba added the enhancement New feature or request label Jul 9, 2021
@vmx
Copy link
Contributor Author

vmx commented Jul 9, 2021

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
Copy link
Owner

kenba commented Jul 9, 2021

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
Copy link
Contributor Author

vmx commented Jul 9, 2021

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.

@vmx vmx closed this as completed Jul 9, 2021
kenba added a commit that referenced this issue Jul 10, 2021
Also add `Sampler` methods for `get_sampler_info`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants