From 67ae3a49e4864133784cd745a9a8866889f3ae15 Mon Sep 17 00:00:00 2001
From: Tobias Bucher <tobiasbucher5991@gmail.com>
Date: Tue, 9 Dec 2014 18:17:18 +0100
Subject: [PATCH 1/2] Clean up `libcollections::VecMap`
- Introduce a named type for the return type of `VecMap::move_iter`
- Rename all type parameters to `V` for "Value".
- Remove unnecessary call to an `Option::unwrap`, use pattern matching instead.
- Remove incorrect `Hash` implementation which took the `VecMap`'s capacity
into account.
This is a [breaking-change], however whoever used the `Hash` implementation
relied on an incorrect implementation.
---
src/libcollections/vec_map.rs | 89 ++++++++++++++---------------------
1 file changed, 35 insertions(+), 54 deletions(-)
diff --git a/src/libcollections/vec_map.rs b/src/libcollections/vec_map.rs
index 36e66ed27f3..cbe48bf45e7 100644
--- a/src/libcollections/vec_map.rs
+++ b/src/libcollections/vec_map.rs
@@ -23,8 +23,6 @@ use core::mem::replace;
use {vec, slice};
use vec::Vec;
-use hash;
-use hash::Hash;
// FIXME(conventions): capacity management???
@@ -62,8 +60,8 @@ use hash::Hash;
/// assert!(months.is_empty());
/// ```
#[deriving(PartialEq, Eq)]
-pub struct VecMap<T> {
- v: Vec<Option<T>>,
+pub struct VecMap<V> {
+ v: Vec<Option<V>>,
}
impl<V> Default for VecMap<V> {
@@ -83,12 +81,6 @@ impl<V:Clone> Clone for VecMap<V> {
}
}
-impl <S: hash::Writer, T: Hash<S>> Hash<S> for VecMap<T> {
- fn hash(&self, state: &mut S) {
- self.v.hash(state)
- }
-}
-
impl<V> VecMap<V> {
/// Creates an empty `VecMap`.
///
@@ -99,7 +91,7 @@ impl<V> VecMap<V> {
/// let mut map: VecMap<&str> = VecMap::new();
/// ```
#[unstable = "matches collection reform specification, waiting for dust to settle"]
- pub fn new() -> VecMap<V> { VecMap{v: vec!()} }
+ pub fn new() -> VecMap<V> { VecMap { v: vec![] } }
/// Creates an empty `VecMap` with space for at least `capacity`
/// elements before resizing.
@@ -207,10 +199,7 @@ impl<V> VecMap<V> {
/// assert_eq!(vec, vec![(1, "a"), (2, "b"), (3, "c")]);
/// ```
#[unstable = "matches collection reform specification, waiting for dust to settle"]
- pub fn into_iter(&mut self)
- -> FilterMap<(uint, Option<V>), (uint, V),
- Enumerate<vec::MoveItems<Option<V>>>>
- {
+ pub fn into_iter(&mut self) -> MoveItems<V> {
let values = replace(&mut self.v, vec!());
values.into_iter().enumerate().filter_map(|(i, v)| {
v.map(|v| (i, v))
@@ -523,16 +512,19 @@ impl<V> IndexMut<uint, V> for VecMap<V> {
macro_rules! iterator {
(impl $name:ident -> $elem:ty, $($getter:ident),+) => {
- impl<'a, T> Iterator<$elem> for $name<'a, T> {
+ impl<'a, V> Iterator<$elem> for $name<'a, V> {
#[inline]
fn next(&mut self) -> Option<$elem> {
while self.front < self.back {
match self.iter.next() {
Some(elem) => {
- if elem.is_some() {
- let index = self.front;
- self.front += 1;
- return Some((index, elem $(. $getter ())+));
+ match elem$(. $getter ())+ {
+ Some(x) => {
+ let index = self.front;
+ self.front += 1;
+ return Some((index, x));
+ },
+ None => {},
}
}
_ => ()
@@ -552,15 +544,18 @@ macro_rules! iterator {
macro_rules! double_ended_iterator {
(impl $name:ident -> $elem:ty, $($getter:ident),+) => {
- impl<'a, T> DoubleEndedIterator<$elem> for $name<'a, T> {
+ impl<'a, V> DoubleEndedIterator<$elem> for $name<'a, V> {
#[inline]
fn next_back(&mut self) -> Option<$elem> {
while self.front < self.back {
match self.iter.next_back() {
Some(elem) => {
- if elem.is_some() {
- self.back -= 1;
- return Some((self.back, elem$(. $getter ())+));
+ match elem$(. $getter ())+ {
+ Some(x) => {
+ self.back -= 1;
+ return Some((self.back, x));
+ },
+ None => {},
}
}
_ => ()
@@ -574,39 +569,42 @@ macro_rules! double_ended_iterator {
}
/// Forward iterator over a map.
-pub struct Entries<'a, T:'a> {
+pub struct Entries<'a, V:'a> {
front: uint,
back: uint,
- iter: slice::Items<'a, Option<T>>
+ iter: slice::Items<'a, Option<V>>
}
-iterator!(impl Entries -> (uint, &'a T), as_ref, unwrap)
-double_ended_iterator!(impl Entries -> (uint, &'a T), as_ref, unwrap)
+iterator!(impl Entries -> (uint, &'a V), as_ref)
+double_ended_iterator!(impl Entries -> (uint, &'a V), as_ref)
/// Forward iterator over the key-value pairs of a map, with the
/// values being mutable.
-pub struct MutEntries<'a, T:'a> {
+pub struct MutEntries<'a, V:'a> {
front: uint,
back: uint,
- iter: slice::MutItems<'a, Option<T>>
+ iter: slice::MutItems<'a, Option<V>>
}
-iterator!(impl MutEntries -> (uint, &'a mut T), as_mut, unwrap)
-double_ended_iterator!(impl MutEntries -> (uint, &'a mut T), as_mut, unwrap)
+iterator!(impl MutEntries -> (uint, &'a mut V), as_mut)
+double_ended_iterator!(impl MutEntries -> (uint, &'a mut V), as_mut)
/// Forward iterator over the keys of a map
-pub type Keys<'a, T> =
- iter::Map<'static, (uint, &'a T), uint, Entries<'a, T>>;
+pub type Keys<'a, V> =
+ iter::Map<'static, (uint, &'a V), uint, Entries<'a, V>>;
/// Forward iterator over the values of a map
-pub type Values<'a, T> =
- iter::Map<'static, (uint, &'a T), &'a T, Entries<'a, T>>;
+pub type Values<'a, V> =
+ iter::Map<'static, (uint, &'a V), &'a V, Entries<'a, V>>;
+
+/// Iterator over the key-value pairs of a map, the iterator consumes the map
+pub type MoveItems<V> =
+ FilterMap<'static, (uint, Option<V>), (uint, V), Enumerate<vec::MoveItems<Option<V>>>>;
#[cfg(test)]
mod test_map {
use std::prelude::*;
use vec::Vec;
- use hash;
use super::VecMap;
@@ -920,23 +918,6 @@ mod test_map {
assert!(a < b && a <= b);
}
- #[test]
- fn test_hash() {
- let mut x = VecMap::new();
- let mut y = VecMap::new();
-
- assert!(hash::hash(&x) == hash::hash(&y));
- x.insert(1, 'a');
- x.insert(2, 'b');
- x.insert(3, 'c');
-
- y.insert(3, 'c');
- y.insert(2, 'b');
- y.insert(1, 'a');
-
- assert!(hash::hash(&x) == hash::hash(&y));
- }
-
#[test]
fn test_from_iter() {
let xs: Vec<(uint, char)> = vec![(1u, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')];
From 20eaf168c5b40c003bd7d28fe4c071b24118410e Mon Sep 17 00:00:00 2001
From: Tobias Bucher <tobiasbucher5991@gmail.com>
Date: Tue, 9 Dec 2014 20:05:51 +0100
Subject: [PATCH 2/2] Add a proper `Hash` implementation for `VecMap`
Also re-add the previously deleted test with an additional test that would have
failed before, when the hash function depended on the capacity.
---
src/libcollections/vec_map.rs | 37 +++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/src/libcollections/vec_map.rs b/src/libcollections/vec_map.rs
index cbe48bf45e7..e2efcb62ba9 100644
--- a/src/libcollections/vec_map.rs
+++ b/src/libcollections/vec_map.rs
@@ -21,6 +21,7 @@ use core::iter;
use core::iter::{Enumerate, FilterMap};
use core::mem::replace;
+use hash::{Hash, Writer};
use {vec, slice};
use vec::Vec;
@@ -81,6 +82,19 @@ impl<V:Clone> Clone for VecMap<V> {
}
}
+impl<S: Writer, V: Hash<S>> Hash<S> for VecMap<V> {
+ fn hash(&self, state: &mut S) {
+ // In order to not traverse the `VecMap` twice, count the elements
+ // during iteration.
+ let mut count: uint = 0;
+ for elt in self.iter() {
+ elt.hash(state);
+ count += 1;
+ }
+ count.hash(state);
+ }
+}
+
impl<V> VecMap<V> {
/// Creates an empty `VecMap`.
///
@@ -605,6 +619,7 @@ pub type MoveItems<V> =
mod test_map {
use std::prelude::*;
use vec::Vec;
+ use hash::hash;
use super::VecMap;
@@ -918,6 +933,28 @@ mod test_map {
assert!(a < b && a <= b);
}
+ #[test]
+ fn test_hash() {
+ let mut x = VecMap::new();
+ let mut y = VecMap::new();
+
+ assert!(hash(&x) == hash(&y));
+ x.insert(1, 'a');
+ x.insert(2, 'b');
+ x.insert(3, 'c');
+
+ y.insert(3, 'c');
+ y.insert(2, 'b');
+ y.insert(1, 'a');
+
+ assert!(hash(&x) == hash(&y));
+
+ x.insert(1000, 'd');
+ x.remove(&1000);
+
+ assert!(hash(&x) == hash(&y));
+ }
+
#[test]
fn test_from_iter() {
let xs: Vec<(uint, char)> = vec![(1u, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')];