search

JuliaIO / HDF5.jl

Save and load data in the HDF5 file format from Julia
https://juliaio.github.io/HDF5.jl
MIT License
380 stars 138 forks source link

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

Closed david-macmahon closed 3 months ago

david-macmahon commented 5 months ago

Fixes #1137

mkitti commented 5 months ago

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 commented 5 months ago

I think we also want to change to Cuchar here: https://github.com/JuliaIO/HDF5.jl/blob/91ef2841dcc5ddd52ef3094120b8921e0c1c5749/gen/api_defs.jl#L30

david-macmahon commented 5 months ago

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 commented 5 months ago

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.

https://github.com/JuliaIO/HDF5.jl/blob/91ef2841dcc5ddd52ef3094120b8921e0c1c5749/gen/gen_wrappers.jl#L2

simonbyrne commented 5 months ago

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

mkitti commented 5 months ago

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 commented 3 months ago

I'm going to try to push this through soon