Minimal Object Reference Implementation #279

JamesMc86 · 2024-04-13T12:57:32Z

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:

Could we store the location in the references as well as it gets passed on nearly every call?
There is a todo in the DynValue that @mulimoen included - I'm not familiar with this to know what to do but I assume a supported type needs adding before release.
ObjectReference2 can't be supported in VarArray due to not implementing a Copy. I would propose adding a TryClone type that VarArray can take so we can support this. I'm happy to do this in another release.
The same basic structure can be used to add Region references and Attribute references in a future version.

Added a trait for object references and refactored the StdReference as a newtype (ObjectReference2) and behind the trait so we can support it side by side with the older object references.

This commit represents an MVP for object references. This includes the implementation of Object1 for 1.8 and later, and Object2 for 1.12 and later.

The wrapper to H5RCopy is causing issues in some enviroments. This commit removes it for now as it isn't critical to the basic object implementation.

There appear to be bugs in the v1.12.0 version which certainly break ObjectReference2 is an element in compound type. Other bugs were seen previously as well so this appears to be a more stable option.

JamesMc86 · 2024-04-14T09:12:32Z

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

This looks great, just move a couple of structs and this should be ready to merge.

mulimoen · 2024-04-14T09:44:19Z

hdf5-types/src/dyn_value.rs

@@ -703,6 +703,7 @@ impl<'a> DynValue<'a> {
            FixedUnicode(_) => DynFixedString::new(buf, true).into(),
            VarLenAscii => DynVarLenString::new(buf, false).into(),
            VarLenUnicode => DynVarLenString::new(buf, true).into(),
+            Reference(_x) => todo!(),


Should we support this?

hdf5/src/hl/references/legacy.rs

hdf5/src/hl/references/standard.rs

mulimoen · 2024-04-14T10:00:34Z

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 · 2024-04-15T10:13:01Z

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 · 2024-04-15T10:16:57Z

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

hdf5-types/src/dyn_value.rs

Co-authored-by: Magnus Ulimoen <mulimoen@users.noreply.github.com> Signed-off-by: James McNally <james@wiresmithtech.com>

JamesMc86 · 2024-05-23T11:18:35Z

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 · 2024-05-23T11:25:12Z

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.

mulimoen and others added 14 commits April 3, 2024 22:09

checkpoint

ff695a6

Working?

c9065db

checkpoint

4737ddc

Add Integration Tests for Current Reference Implementations

c01e92d

Factor references out to their own module

67fe689

Add version flags to HDF5-Types References

b06368a

refactor: ObjectReference Trait

98a8e2b

Added a trait for object references and refactored the StdReference as a newtype (ObjectReference2) and behind the trait so we can support it side by side with the older object references.

fix: Have types work against pre-1.12

c41b474

wip: Support for Object1 and Object2 References

3688fea

This commit represents an MVP for object references. This includes the implementation of Object1 for 1.8 and later, and Object2 for 1.12 and later.

fix: Mark DynValue variable as unused

f6c0564

fix: Remove StdReference try_clone

36fb026

The wrapper to H5RCopy is causing issues in some enviroments. This commit removes it for now as it isn't critical to the basic object implementation.

refactor: some renames

4cb6c13

refactor: Move References to Module in H5Types

2d70d84

fix: Gate ObjectReference2 on v1.12.1

425f187

There appear to be bugs in the v1.12.0 version which certainly break ObjectReference2 is an element in compound type. Other bugs were seen previously as well so this appears to be a more stable option.

JamesMc86 changed the title ~~Minimal Object Reference Implementation~~ Draft: Minimal Object Reference Implementation Apr 14, 2024

JamesMc86 added 2 commits April 14, 2024 10:14

backout VarArray non-copy changes

24ab51d

fix: Fix API compatibility for ObjectReference1

b975a21

mulimoen requested changes Apr 14, 2024

View reviewed changes

mulimoen reviewed Apr 20, 2024

View reviewed changes

hdf5-types/src/dyn_value.rs Outdated Show resolved Hide resolved

mulimoen approved these changes Apr 20, 2024

View reviewed changes

JamesMc86 changed the title ~~Draft: Minimal Object Reference Implementation~~ Minimal Object Reference Implementation Apr 21, 2024

Update hdf5-types/src/dyn_value.rs

fdb5edc

Co-authored-by: Magnus Ulimoen <mulimoen@users.noreply.github.com> Signed-off-by: James McNally <james@wiresmithtech.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal Object Reference Implementation #279

Minimal Object Reference Implementation #279

JamesMc86 commented Apr 13, 2024

JamesMc86 commented Apr 14, 2024

mulimoen left a comment

mulimoen Apr 14, 2024

mulimoen commented Apr 14, 2024

JamesMc86 commented Apr 15, 2024

mulimoen commented Apr 15, 2024

JamesMc86 commented May 23, 2024

mulimoen commented May 23, 2024

Minimal Object Reference Implementation #279

Are you sure you want to change the base?

Minimal Object Reference Implementation #279

Conversation

JamesMc86 commented Apr 13, 2024

JamesMc86 commented Apr 14, 2024

mulimoen left a comment

Choose a reason for hiding this comment

mulimoen Apr 14, 2024

Choose a reason for hiding this comment

mulimoen commented Apr 14, 2024

JamesMc86 commented Apr 15, 2024

mulimoen commented Apr 15, 2024

JamesMc86 commented May 23, 2024

mulimoen commented May 23, 2024