Skip to content

Adds a OrderMap::drain() method #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 29, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
target
Cargo.lock
58 changes: 58 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method moved.

/// Keeps the allocated memory for reuse.
pub fn drain(&mut self) -> Drain<K, V> {
for i in &mut self.indices {
*i = Pos::none();
}

Drain {
inner: self.entries.drain(..),
}
}
}

/// Trait for the "size class". Either u32 or u64 depending on the index
Expand Down Expand Up @@ -1496,6 +1508,17 @@ impl<K, V, S> Eq for OrderMap<K, V, S>
{
}

pub struct Drain<'a, K, V> where K: 'a, V: 'a {
inner: ::std::vec::Drain<'a, Bucket<K, V>>
}

impl<'a, K, V> Iterator for Drain<'a, K, V> {
type Item = (K, V);
fn next(&mut self) -> Option<Self::Item> {
self.inner.next().map(|bucket| (bucket.key, bucket.value))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added by delegating to inner.size_hint()


#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -1690,4 +1713,39 @@ mod tests {
assert_ne!(map_a, map_c);
assert_ne!(map_c, map_a);
}


#[test]
fn drain_basic() {
let mut map_a = OrderMap::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

map_a.insert(1, "1");
map_a.insert(2, "2");
let entries: Vec<(u32, &str)> = map_a.drain().collect();
assert!(map_a.is_empty());
assert_eq!(entries.len(), 2);
assert!(map_a.is_empty());

map_a.insert(3, "3");
assert!(!map_a.is_empty());
assert_eq!(map_a.len(), 1);
assert!(map_a.get(&3).is_some());
assert!(map_a.get(&1).is_none());
}

#[test]
fn drain_after_resize() {
let mut map_a = OrderMap::with_capacity(2);
map_a.insert(1, "1");
map_a.insert(2, "2");
map_a.insert(3, "3");
let entries: Vec<(u32, &str)> = map_a.drain().collect();
assert!(map_a.is_empty());
assert_eq!(entries.len(), 3);

map_a.insert(4, "4");
assert!(!map_a.is_empty());
assert_eq!(map_a.len(), 1);
assert!(map_a.get(&4).is_some());
assert!(map_a.get(&1).is_none());
}
}