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

Update helpers.jl with initial value for Ref{Cuint} #1138

Merged
merged 3 commits into from
Mar 26, 2024

Conversation

david-macmahon
Copy link
Contributor

Fixes #1137

@mkitti
Copy link
Member

mkitti commented Jan 19, 2024

This is supposed to be a C99 bool aka _Bool. The appropriate type for the Ref should be Cuchar.

https://docs.hdfgroup.org/hdf5/develop/group___h5.html#ga70bfde4acd009cdd7bcd2f54c594e28a

@mkitti
Copy link
Member

mkitti commented Jan 19, 2024

I think we also want to change to Cuchar here:

@bind h5_is_library_threadsafe(is_ts::Ref{Cuint})::herr_t "Error determining thread safety"

@david-macmahon
Copy link
Contributor Author

Yeah, I didn't realize/forgot that a lot of the api files are generated. I guess the generator doesn't provide an initial value when generating so it relies on having the type size correct. Maybe best to change it to is_ts = Ref{Cchar}() in helpers.jl and Cuchar in gen_api.jl. I can make the changes, but I don't have cycles to test them. Does CI regenerate the api files? I guess we should add a test for this, but how does one test something that relies on an uninitialized value being (or not being) a certain value?

@mkitti
Copy link
Member

mkitti commented Jan 20, 2024

Once you change api_defs.jl, just run the gen_wrappers.jl script as indicated below. If you cannot, I would be happy to do it.

# Run `julia --project=.. gen_wrappers.jl`` to execute this script

@simonbyrne
Copy link
Collaborator

Maybe we should switch to Clang.jl-generated wrappers.

@mkitti
Copy link
Member

mkitti commented Jan 20, 2024

I worked on that a few years ago in LibHDF5.jl:
https://github.com/mkitti/LibHDF5.jl/blob/main/src/bind.jl

In particular, this line:
https://github.com/mkitti/LibHDF5.jl/blob/712b6e306a15de37f748727b37676aca70ea0664/src/bind.jl#L434

And this line would have lead to the correct definition:
https://github.com/mkitti/LibHDF5.jl/blob/712b6e306a15de37f748727b37676aca70ea0664/src/bind.jl#L192

The problem is that we also have these custom error messages as well.

@mkitti
Copy link
Member

mkitti commented Mar 26, 2024

I'm going to try to push this through soon

@mkitti mkitti merged commit 3fe1936 into JuliaIO:master Mar 26, 2024
19 of 20 checks passed
mkitti added a commit that referenced this pull request Apr 5, 2024
* Update helpers.jl with initial value for Ref{Cuint}

Fixes #1137

* Switch to using Cuchar

---------

Co-authored-by: Mark Kittisopikul <[email protected]>
Co-authored-by: Mark Kittisopikul <[email protected]>
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.

h5_is_library_threadsafe() gives unreliable results due to unspecified initial value
3 participants