Add truncate and retain(_mut) to Deque #617
Conversation
src/deque.rs
Outdated
| // Stage 1: Check if all values can be retained. | ||
| while cur < len { | ||
| // Safety: `cur` must be under the deque's total length as per the loop condition | ||
| if !f(unsafe { self.get_unchecked_mut(cur) }) { |
There was a problem hiding this comment.
Why deviate from std's implementation and use unsafe here? I expect the bound check introduced in the safe path to be optimized out.
There was a problem hiding this comment.
Fair! My reasoning came from the fact that Deque doesn't impl ops::IndexMut (and most things in heapless seem to not, I'm unsure how intentional of a choice that is still). I like to avoid unwraps in unsafe blocks for a few reasons; expect is fine, but I also felt it was unnecessarily explicit? I mean, std::VecDeque's IndexMut impl comes out to be the same as what I have now changed it to, a get_mut() with an except().
So maybe another PR adding those ops impls would be in order? Unless this was an explicit choice to not allow indexing like that, I'm still new to heapless as a whole 😅.
src/deque.rs
Outdated
| // Based on alloc's own VecDeque test | ||
| // | ||
| // https://github.com/rust-lang/rust/blob/fa3155a644dd62e865825087b403646be01d4cef/library/alloc/src/collections/vec_deque/tests.rs#L1086 | ||
| // | ||
| // Tests that each element's destructor is called when being truncated. |
There was a problem hiding this comment.
Please test the following cases:
- Truncating an empty deque
- Truncating an continuous deque
- leaving it unchanged
- leaving it empty
- leaving it half-empty
- Truncating a discontinuous deque
- leaving it unchanged
- leaving it discontinuous but dropping some elements
- leaving it continuous (i.e. all the back is dropped)
- reducing the front slice
- not touching the front slice
- leaving it empty
Essentially ensure that all branches of the truncate function are exercized.
There was a problem hiding this comment.
Added more tests, thanks for the suggestions! I was surprised there wasn't more in the std, maybe I just didn't look deep enough.
Let me know if any should be adjusted.
|
@sgued Let me know how you'd like me to proceed in regards to the outstanding conversations (about |
|
It looks fine sorry. |
sgued
force-pushed
the
deque_truncate_retain
branch
from
f3087f6 to
63fcb52
Compare
sgued
force-pushed
the
deque_truncate_retain
branch
from
dc6a355 to
fbe5f3f
Compare
2 participants
Howdy!
As I was working on simplifying a crate's logic to fit in a
no_stdcontext, I found thatDequedidn't have some of the methods I was using fromalloc's implementation.So I took the time to port them over! Apologies if the comments in
truncateare a bit much, it was mostly to help me wrap my head around how the data structure itself used its private variables.If you spot an error in my logic or if I missed something, please let me know!