Add method .remove_index(axis, index) #967
Conversation
|
This is my approach. Safe version (commit 1) and optimized (commit 2) are not so far apart, and they should be pretty good and remarkably simple as a first approach. They don't change the number of initialized elements, just like slicing in place works. |
src/impl_methods.rs
Outdated
| let mut slot = MaybeUninit::<A>::uninit(); | ||
| unsafe { | ||
| slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); | ||
| for elt in lane_iter { |
There was a problem hiding this comment.
If lane_iter.next() panics, then an element will be dropped twice, and, less importantly, the element in slot won't be dropped. I don't think the current implementation can panic, but this still makes me uncomfortable. I'd feel more comfortable with getting a raw pointer to the start of the lane, the stride for the axis, and then .offset()ting the pointer by the stride on each iteration, because this would be easier to audit for potential panics.
There was a problem hiding this comment.
Yes I know this is a no-panic section and it should be marked. I was comfortable apparently, but I had a thought to maybe leave an "abort on panic" guard here to make it entirely visible. (What do you think?)
As for a simpler iteration, in that case we should implement a special 1D iterator that does just that (even though Zip is already doing about the same job). I don't want to write out a raw pointer loop, ndarray probably needs a bit more use of higher level abstractions, not less 🙂
src/impl_methods.rs
Outdated
| let mut slot = MaybeUninit::<A>::uninit(); | ||
| unsafe { | ||
| slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); | ||
| for elt in lane_iter { |
There was a problem hiding this comment.
IMO, src would be a clearer name than elt.
There was a problem hiding this comment.
Hm matter of taste that I haven't addressed
src/impl_methods.rs
Outdated
| // use swap to keep all elements initialized (as required by owned storage) | ||
| Zip::from(tail.lanes_mut(axis)).for_each(|mut lane| { | ||
| let mut lane_iter = lane.iter_mut(); | ||
| let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; |
There was a problem hiding this comment.
If we panic on the index == self.len_of(axis) case as I suggested above, the None case cannot occur.
There was a problem hiding this comment.
Yes. Type system wise the existing code is neat enough, don't know if anything else is worthwhile?
bluss
changed the title
bluss
force-pushed
the
shift-remove
branch
2 times, most recently
from
497169e
to
5b6b215
Compare
Co-authored-by: Jim Turner <[email protected]>
Removing the index-past-the end does not make sense (it's an ok split index, so the previous code passed through without action).
It's a critical section where we can't panic. Mark the reason in the guard and message.
Add method to remove the
indexth entry along all lanes ofaxis.The method doesn't change the number of initialized elements, just like slicing in place works
(this could be confusing, w.r.t remove semantics).
Fixes #931