Skip to content

Add more iterators on Group#157

Merged
aldanor merged 24 commits into
aldanor:masterfrom
mulimoen:feature/iteration
Oct 13, 2021
Merged

Add more iterators on Group#157
aldanor merged 24 commits into
aldanor:masterfrom
mulimoen:feature/iteration

Conversation

@mulimoen

@mulimoen mulimoen commented Jun 4, 2021

Copy link
Copy Markdown
Collaborator

This adds groups, datasets, and datatypes to Group. This also adds the general iter_visit, which can be adapted by users to fully use the capabilities of H5Literate (sans iteration resumption).

@mulimoen
mulimoen force-pushed the feature/iteration branch 3 times, most recently from d9587cd to dfd4c71 Compare June 4, 2021 20:57
@aldanor aldanor mentioned this pull request Oct 6, 2021
@mulimoen

mulimoen commented Oct 6, 2021

Copy link
Copy Markdown
Collaborator Author

@aldanor This should be good to go now

@aldanor

aldanor commented Oct 6, 2021

Copy link
Copy Markdown
Owner

@mulimoen Looks good overall. I started writing review but figured it would be easier to fix myself - I'll open a PR to your PR on your fork.

@aldanor

aldanor commented Oct 6, 2021

Copy link
Copy Markdown
Owner

One minor point - why the manual .incref() on the group handle? (otherwise, it looks similar to ObjectClass::from_id() constructor)

@mulimoen

mulimoen commented Oct 6, 2021

Copy link
Copy Markdown
Collaborator Author

hdf5 has ownership of this ID, and we need to ensure we don't invalidate the id before we return it, as there is an unconditional close on the ID after the callback. This is where #139 would be useful: We only have a borrowed handle and need to treat it as such

@aldanor

aldanor commented Oct 11, 2021

Copy link
Copy Markdown
Owner

For reference: mulimoen#11

@mulimoen

Copy link
Copy Markdown
Collaborator Author

@aldanor Looks good to me now. I forgot how big this release is, I thought we already published attributes, but apparently not

@aldanor

aldanor commented Oct 13, 2021

Copy link
Copy Markdown
Owner

Hm, with times not being filled out, that's not good - but I'll open a separate PR to quickly fix that. This one - let's merge then.

Comment thread src/hl/group.rs
}
}

pub enum IterationOrder {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[derive(Clone, Copy, Debug, PartialEq, Eq)]?

Comment thread src/hl/group.rs
) -> herr_t {
panic::catch_unwind(|| {
let other_data: &mut Vec<String> = unsafe { &mut *(op_data.cast::<Vec<String>>()) };
pub enum TraversalOrder {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[derive(Clone, Copy, Debug, PartialEq, Eq)]?

Comment thread src/hl/group.rs
let name = unsafe { std::ffi::CStr::from_ptr(name) };
let info = unsafe { info.as_ref().unwrap() };
let handle = Handle::try_new(id).unwrap();
handle.incref();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could probably do Group::from_id(id).expect("invalid group id"), no?

The only difference would be it doesn't incref the handle there, wonder why it's increfed manually here?

Comment thread src/hl/group.rs Outdated
let vtable = op_data.cast::<Vtable<F, G>>();
let vtable = unsafe { vtable.as_mut().expect("op_data is always non-null") };
let name = unsafe { std::ffi::CStr::from_ptr(name) };
let info = unsafe { info.as_ref().unwrap() };

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace all unwraps within ffi code with .expect()?

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