aldanor / hdf5-rust

HDF5 for Rust
https://docs.rs/hdf5
Apache License 2.0
308 stars 82 forks source link

Minimal Object Reference Implementation #279

Open JamesMc86 opened 5 months ago

JamesMc86 commented 5 months ago

This PR includes changes to support object references as requested in #98.

It adds ObjectReference1 as pre-1.12 object references and ObjectReference2 as the new reference types based on the STD_REF type.

I've tried to make this useful without being too large and can build on it further.

Key questions for me are around the terminology as I'm quite new to HDF5 - let me know if that makes sense.

Other possible issues to consider:

JamesMc86 commented 5 months ago

Obviously got into a bit of a mess here still so will mark as draft. I thought I had it.

One problem is bugs in 1.12.0 with references so now flagged as 1.12.1 required.

Another is with the VarLenArray I believe. Going to back out the changes to make them work with Non-Copy types and the ObjectReference2 will just not be supported in them at this point.

I've successfully got conda replicating another issue on 1.8 so I'll work on that too!

mulimoen commented 5 months ago

I think we can lift the restriction of Copy on H5Type if we require derived types to be Copy. But this depends on what sort of issues we got for VarLenArray<ref> which could suggest something worse going on.

JamesMc86 commented 5 months ago

I think we can lift the restriction of Copy on H5Type if we require derived types to be Copy. But this depends on what sort of issues we got for VarLenArray<ref> which could suggest something worse going on.

Yes I think there is a way - the main issue is the ptr lifetime and implementing our own drop routine. I've been trying to think if there is a way to treat it as a slice so we get some for free but I guess it will always have the problem of needing to be C compatible with the HDF5 library.

I'm happy to have a go with this as well but didn't want to grow this PR any bigger!

mulimoen commented 5 months ago

I'm more than OK with keeping this PR small. We have #232 which attempts to solve the Copy problem.

JamesMc86 commented 4 months ago

Just wanted to check what was blocking this and #243 as they will start blocking my project. Happy to do work here to unblock it. I see the falling brew install was removed in another PR anyway. I can try and triage the MinGW issue too

mulimoen commented 4 months ago

I would like an approval from @aldanor, but for me this is looking good to go. Note that only @aldanor has rights to release as of now (and a release is long overdue).

It would be great if you managed to track down the mingw error, it is present also on master so could be a separate PR.