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

Initialize properties on get. #993

Merged
merged 3 commits into from
Jul 19, 2022

Conversation

mkitti
Copy link
Member

@mkitti mkitti commented Jul 19, 2022

Also add H5P_ATTRIBUTE_ACCESS constant.

@mkitti mkitti changed the title Initialize property on get. Initialize properties on get. Jul 19, 2022
@mkitti
Copy link
Member Author

mkitti commented Jul 19, 2022

Currently, trying to access a property of a property list without calling HDF5.init! will fail with an error.

julia> using HDF5

julia> dcpl = HDF5.DatasetCreateProperties()
HDF5.DatasetCreateProperties()

julia> HDF5.API.SHORT_ERROR[] = false
false

julia> dcpl.alloc_time
ERROR: HDF5.API.H5Error: Error getting allocation timing
libhdf5 Stacktrace:
 [1] H5P_isa_class: Invalid arguments to routine/Inappropriate type
     not a property list
 [2] H5P_object_verify: Property lists/Unable to register new atom
     property list is not a member of the class
 [3] H5Pget_alloc_time: Object atom/Unable to find atom information (already closed?)
     can't find object for ID
Stacktrace:
 [1] macro expansion
   @ C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\error.jl:18 [inlined]
 [2] h5p_get_alloc_time(plist_id::HDF5.DatasetCreateProperties, alloc_time::Base.RefValue{Int32})
   @ HDF5.API C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\functions.jl:1579
 [3] h5p_get_alloc_time(plist_id::HDF5.DatasetCreateProperties)
   @ HDF5.API C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\api\helpers.jl:425
 [4] get_alloc_time(p::HDF5.DatasetCreateProperties)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:185
 [5] class_getproperty(#unused#::Type{HDF5.DatasetCreateProperties}, p::HDF5.DatasetCreateProperties, name::Symbol)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:498
 [6] getproperty(p::HDF5.DatasetCreateProperties, name::Symbol)
   @ HDF5 C:\Users\kittisopikulm\.julia\packages\HDF5\7zvRl\src\properties.jl:38
 [7] top-level scope
   @ REPL[28]:1

This change will ensure that property lists are initialized when someone tries to retrieve a property.

julia> using HDF5
[ Info: Precompiling HDF5 [f67ccb44-e63f-5c2f-98bd-6dc0ccc4ba2f]

julia> dcpl = HDF5.DatasetCreateProperties()
HDF5.DatasetCreateProperties()

julia> dcpl.id
0

julia> dcpl.alloc_time
:late

julia> dcpl.id
792633534417207321

By incorporating init! into getproperty we do introduce a isvalid check on each property access call.

function init!(prop::P) where {P<:Properties}
if !isvalid(prop)
prop.id = API.h5p_create(classid(P))
end
return prop
end

@mkitti
Copy link
Member Author

mkitti commented Jul 19, 2022

I also noticed that we were missing the H5P_ATTRIBUTE_ACCESS property constant value.

@mkitti
Copy link
Member Author

mkitti commented Jul 19, 2022

A shorter version of isvalid would be to check if the id field is -1 or 0 rather than possibly calling HDF5.API.h5i_is_valid on each check.

Given that this is a high-level API, I think the additional auto-initialization is worth while. Performance sensitive applications can use the mid-level or low-level API to get around such checks.

@musm
Copy link
Member

musm commented Jul 19, 2022

I don't think the isvalid is that big an issue. Correctness is certainly preferred, plus we have those isvalid checks throughout the codebase in most accessors.

@musm musm merged commit fb37b2e into JuliaIO:master Jul 19, 2022
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