Accumulate methods #513
Accumulate methods #513
Conversation
There was a problem hiding this comment.
As I alluded to in #506, getting this right is tricky. Whenever there's an uninitialized instance of a type that may implement Drop, we have to be extremely careful not to panic until it's initialized, because a panic results in dropping it (which is undefined behavior). This means that we have to audit everything between when an uninitialized instance is created and when it is fully initialized to make sure nothing ever panics, and we have to make sure that even if nothing panics in the current implementation, that stays true in the future. (See the first example for ::std::mem::uninitialized() and in particular the "DANGER ZONE".)
As soon as #496 is merged, this will be much simpler to implement correctly. (Create the uninitialized Vec, create an ArrayPtrMut pointing at it, ::std::mem::forget the Vec (so we no longer have to worry about panics causing undefined behavior), perform the initialization (using ptr::write for writing elements), reconstruct the Vec using ::std::vec::Vec::from_raw_parts, and finally convert it into an Array.)
I have a few things left to work out before I merge #496. Once it's merged, we can update this PR to take advantage of ArrayPtrMut.
| v.set_len(self.len()); | ||
| Array::<A, D>::from_shape_vec_unchecked(self.dim(), v) | ||
| }; | ||
| let mut states = self.subview(axis, 0).to_owned(); |
There was a problem hiding this comment.
This results in undefined behavior if A implements Drop and the length of axis axis is zero. (If the length of the axis is zero, this line will panic, which will cause the uninitialized array accum to be dropped, which in turn will cause each of the uninitialized elements to be dropped (which is undefined behavior).)
| Zip::from(&mut accum_i) | ||
| .and(&mut states) | ||
| .and(&self_i) | ||
| .apply(|ac, st, se| *ac = f(st, se)); |
There was a problem hiding this comment.
This results in undefined behavior if B implements Drop, because ac is being assigned without using ptr::write, so its old (uninitialized value) gets dropped. We also have to worry about f panicking, because a panic will cause accum to be dropped, which will cause each of its elements to be dropped, including uninitialized elements.
| /// Inplace version of `accumulate_axis`. See that method for more | ||
| /// documentation. | ||
| /// | ||
| /// **panics** if the dimension of `self` along `axis` is 0. |
There was a problem hiding this comment.
It's not necessary to panic in this case. We can add an explicit check for this case and return an empty array of the correct shape.
| Zip::from(&mut accum_i) | ||
| .and(&mut states) | ||
| .and(&self_i) | ||
| .apply(|ac, st, se| {*st = f(st, se); *ac = st.clone();}); |
There was a problem hiding this comment.
This results in undefined behavior if A implements Drop, because ac is being assigned without using ptr::write, so its old (uninitialized value) gets dropped. We also have to worry about f panicking, because a panic will cause accum to be dropped, which will cause each of its elements to be dropped, including uninitialized elements.
|
Thanks for this, I'll close the PR for now, but feel free to reopen if thre are new developments! We're working on getting better fundamental building blocks in the crate to deal with uninit memory and other tools needed to implement things like this. |
Implements #506 .