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

Include stdbool.h in API headers #3

Closed
wants to merge 1 commit into from

Conversation

FranklandJack
Copy link
Contributor

@FranklandJack FranklandJack commented Nov 9, 2022

  • ur_api.h makes use of bool. ur_api.h is a C header and in C the bool type is defined in stdbool.h, hence we should include ur_api.h in the API header.

@bashbaug
Copy link

bashbaug commented Nov 9, 2022

Alternatively, perhaps we should rethink whether we use bool in the unified runtime. Many APIs use a different explicitly sized type instead, e.g. cl_bool, VkBool32, ze_bool_t, to avoid potentially different sizes for bool.

@FranklandJack
Copy link
Contributor Author

FranklandJack commented Nov 10, 2022

Alternatively, perhaps we should rethink whether we use bool in the unified runtime. Many APIs use a different explicitly sized type instead, e.g. cl_bool, VkBool32, ze_bool_t, to avoid potentially different sizes for bool.

Thanks for your feedback @bashbaug . Good point, I did think it was a little odd the API was using bool and I think what you've suggested is a better solution.

More generally, APIs in the UR spec are using types from stdint.h for things like counts e.g. the numEventsInWaitList uint32_t parameter in urEnqueueMemBufferCopy. Other APIs such as OpenCL would use a API specific typedef here (for example clEnqueueCopyBuffer uses a cl_uint for its num_events_in_wait_list parameter, which is defined to always be 32 bits I think). Do you think we should introduce unified runtime typedefs for these fixed width integer types too? e.g. ur_uint. I think this is less of an issue than bool since the stdint.h types are of definite width already, but for consistencies sake maybe we want analogous integers for the other widths?

* `ur_api.h` makes use of `bool`. `ur_api.h` is a C header and in C
  the `bool` type is defined in `stdbool.h`, hence we should include
  `ur_api.h` in the API header.
@FranklandJack
Copy link
Contributor Author

Closing in favor of #42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants