aldanor / hdf5-rust

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

Enable `T` in `VarLenArray<T>` to also contain a `VarLenArray` #232

Open AlanRace opened 1 year ago

AlanRace commented 1 year ago

This fixes #222.

Additionally, I was implementing a custom filter and required access to the h5err macro, hence the changes to the macro exporting.

aldanor commented 1 year ago

This PR contains tons of unrelated changes and will need rebasing and cleaning, however that's not even the main issue here.

You can't simply change T: Copy to T: Clone as this will be inherently unsafe. The code that's currently in master operates based on the assumption that T cannot implement Drop (because Copy and Drop are mutually exclusive). Consequently, in VarLenArray::<T>::drop(), there's no inner destructors being called, the memory is just being freed at once, which would lead to problems if T is not Copy. Additionally, just allowing T: Clone would allow you stuffing all kinds of scary Rust types into this which would make it even more unsafe.

One way to restrict it, I guess, would be adding something like

pub trait VarLenElement: 'static + Clone {}
impl<T: Copy> VarLenElement for T {}
impl<T: VarLenElement> VarLenElement for VarLenArray<T> {}
impl VarLenElement for VarLenString {}

and then require T: VarLenElement in VarLenArray<T>. In the destructor of VarLenArray, you'd have to manually drop all of the inner elements then.

And then it would need some tests to make sure it actually works.