Adds a OrderMap::drain() method #27
Conversation
|
I'd probably try to avoid requiring the allocations as part of I'd suggest adding a new struct Drain<'a, K, V> {
inner: std::vec::Drain<'a, Bucket<K, V>>,
}Then, impl I'd probably also try to avoid the |
Problem: Porting some code that used HashMap's drain to OrderMap revealed a small API gap. Solution: Added a drain method, mirroring std::collection::HashMap's drain method. drain() provides an efficient way to remove all values without shrinking the capacity of the original OrderMap. Validation: Added unit tests and ported an existing HashMap::drain() user to OrderMap::drain
|
Thank you, Carl! I rewrote the code to address those concerns. Please let me know if I made a mistake, I haven't implemented that many Iterators in Rust. |
There was a problem hiding this comment.
Thanks both, it looks good this way.
OrderMap is an indexed hash map but I'm not sure we should hold this up for that, because we need to do some complicated index shifting if we remove a range.
Ideally we should implement an indexed drain, that follows the trajectory that the data structure is on. However it is also not receiving much of my time so I wouldn't have time to review something more involved.
What do you think of introducing this with a mandatory .. as argument? It would be a weird placeholder inbetween.
src/lib.rs
Outdated
| @@ -400,6 +400,18 @@ impl<K, V, S> OrderMap<K, V, S> | |||
| pub fn capacity(&self) -> usize { | |||
| usable_capacity(self.raw_capacity()) | |||
| } | |||
|
|
|||
| /// Clears the `OrderMap`, returning all key-value pairs as a `Drain`. | |||
There was a problem hiding this comment.
Iterators don't really have a type-identity, they are peculiar that way. The struct is just a vehicle for the implementation, so we don't call it a Drain but maybe a drain iterator.
I think that the method should move down the method list to be placed at a location that degrades it in importance and where users more expect it... after the other iterator methods.
src/lib.rs
Outdated
| fn next(&mut self) -> Option<Self::Item> { | ||
| self.inner.next().map(|bucket| (bucket.key, bucket.value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add size_hint, it's the next important iterator method. If the other OrderMap iterators are exact size iterators, this one should be too. I'm not too exited for any other iterator method implementations, we don't necessarily need those.
There was a problem hiding this comment.
Added by delegating to inner.size_hint()
src/lib.rs
Outdated
|
|
||
| #[test] | ||
| fn drain_basic() { | ||
| let mut map_a = OrderMap::new(); |
There was a problem hiding this comment.
(Not very actionable comment)
Quickcheck tests are always better than example tests. In this case we reuse a solid std iterator and we don't really need any particular test.
There was a problem hiding this comment.
I've replaced the example unit tests with a drain quickcheck test. This is my first time using Rust's quickcheck, please let me know if you think I made some newbie error.
|
@bluss the main problem w/ taking an argument right now is that |
|
unstable RangeArgument is not a problem. Simply provide an equivalent trait. (Arrayvec does this). |
|
Even less of a problem is doing the simple stopgap solution which is to take a single |
* Replaced unit tests with quickcheck tests. * Added a size_hint method. * drain now takes a `..` argument.
|
Let me know if you'd rather I squashed this into a single logical commit. |
|
Thanks! |
|
This checks off an item on HashMap consistency list but adds another (That's my meddling). |
Problem:
Porting some code that used HashMap's drain to OrderMap revealed a small
API gap.
Solution:
Added a drain method, mostly mirroring what std::collections::HashMap
offers except returning a standard Vec instead of a Drain.
drain() provides an efficient way to remove all values without shrinking
the capacity of the original OrderMap.
Validation:
Added unit tests and ported an existing HashMap::drain() user to
OrderMap::drain