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

WIP: Add ability to automatically box scalars in wrapped API with special case argument type #760

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Dec 6, 2020

This is the follow-up to discussions in #759 on how to automatically allow passing scalars through to pointer arguments to make using the low-level API more ergonomic.

Rather than the Ref{Any} type discussed in the comments, though, I instead chose to special case Ref{Cvoid} since Ref{Any} already has a meaning in Julia:

help?> Ref
...
  As a special case, setting `T = Any` will instead cause the creation of a pointer
  to the reference itself when converted to a `Ptr{Any}` (a `jl_value_t const* const*`
  if `T` is immutable, else a `jl_value_t *const *`). When converted to a
  `Ptr{Cvoid}`, it will still return a pointer to the data region as for any other `T`.
...

It's possible that these types of arguments may be required in setting up callback functions which pass through an arbitrary data bundle. On the other hand, Ref{Cvoid} seems to be an error, so there shouldn't be any code that actually needs this data type:

julia> Base.cconvert(Ref{Cvoid}, Ref(1))
ERROR: MethodError: Cannot `convert` an object of type 
  Base.RefValue{Int64} to an object of type 
  Union{}

I've included "WIP:" in the title because there might be some existing cases which could be simplified with this new feature. For instance, it might make sense on the h5d_write and h5a_write methods, which would allow removing some write_(attribute|dataset) specializations if the wrapper can do the boxing automatically. (Just an idea at this point, though — haven't tried actually doing it yet, so there might be a corner case I'm not currently thinking of.)

@jmert
Copy link
Contributor Author

jmert commented Dec 6, 2020

Oh, also with this PR:

julia> using HDF5

julia> b = create_datatype(HDF5.H5T_ENUM, sizeof(Bool))
       HDF5.h5t_enum_insert(b, "FALSE", false)
       HDF5.h5t_enum_insert(b, "TRUE", true)
       b
HDF5.Datatype: H5T_ENUM {
      H5T_STD_I8LE;
      "FALSE"            0;
      "TRUE"             1;
   }

and the explicitly-wrapped case continues to work as is now possible on master:

julia> b′ = create_datatype(HDF5.H5T_ENUM, sizeof(Bool))
       HDF5.h5t_enum_insert(b′, "FALSE", Ref(false))
       HDF5.h5t_enum_insert(b′, "TRUE", Ref(true))
       b′
HDF5.Datatype: H5T_ENUM {
      H5T_STD_I8LE;
      "FALSE"            0;
      "TRUE"             1;
   }

@musm
Copy link
Member

musm commented Dec 6, 2020

LGTM

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