-
Notifications
You must be signed in to change notification settings - Fork 104
Implement H5Type for complex #210
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
Conversation
8a6bd35 to
0cf75d0
Compare
0cf75d0 to
6b791f9
Compare
|
|
||
| unsafe impl<T: H5Type> H5Type for Complex<T> { | ||
| fn type_descriptor() -> TypeDescriptor { | ||
| TypeDescriptor::Compound(CompoundType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inherently unsafe, how do we know that offsets are right? (e.g. should we check actual offsets here?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how we would check the offsets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mulimoen Like this, I think? https://doc.rust-lang.org/stable/std/ptr/macro.addr_of.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would require us to make something to point at. We could make an instance of MaybeUninit, but field access is only a future feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I guess we could do something like this, does that make sense? https://rust.godbolt.org/z/TWbavj98G
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do
unsafe impl<T: H5Type> H5Type for Complex<T> {
fn type_descriptor() -> TypeDescriptor {
use std::mem::MaybeUninit;
// MaybeUninit has the same size and is transparent, but we can
// do some pointer arithmetics on a real instance
let dummystruct: Complex<MaybeUninit<T>> =
Complex::new(MaybeUninit::uninit(), MaybeUninit::uninit());
assert_eq!(size_of_val(&dummystruct), size_of::<Complex<T>>());
let base = addr_of!(dummystruct).cast::<u8>();
let ptr_r = addr_of!(dummystruct.re).cast::<u8>();
let ptr_i = addr_of!(dummystruct.im).cast::<u8>();
TypeDescriptor::Compound(CompoundType {
fields: vec![
// Compatible with h5py definition of complex
CompoundField::typed::<T>("r", unsafe { ptr_r.offset_from(base) } as usize, 0),
CompoundField::typed::<T>("i", unsafe { ptr_i.offset_from(base) } as usize, 1),
],
size: size_of::<Complex<T>>(),
})
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be enough to link to the documentation on representation? https://docs.rs/num-complex/0.4.2/num_complex/struct.Complex.html#representation-and-foreign-function-interface-compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add this link in a comment in code? E.g. "Complex<T> should be FFI-equivalent to [T; 2]: ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added this as a comment under type_descriptor()
6b791f9 to
2a57059
Compare
|
Minor nits aside, needs to be rebased on master (because of the changelog). |
0fd6a0d to
6031bb8
Compare
Closes #208