From 21f4dfdc81b29dc0e86aa70879011528b8ca09aa Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 24 Sep 2017 19:38:54 +0200 Subject: [PATCH 1/2] FEAT: Add trait MutableKeys - Remove get_pair, and rename get_pair_index to get_full. Same with the mutable variant. - Move mutable key methods to MutableKeys -- we want this feature to be out of the way, but opt-in. The trait has mutable key-versions of get_full and retain --- src/lib.rs | 71 +++++++---------------------------------- src/mutable_keys.rs | 77 +++++++++++++++++++++++++++++++++++++++++++++ src/util.rs | 2 ++ 3 files changed, 90 insertions(+), 60 deletions(-) create mode 100644 src/mutable_keys.rs diff --git a/src/lib.rs b/src/lib.rs index 9b6da79efc..53fe7fd6f0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,6 +6,7 @@ mod macros; mod serde; mod util; mod equivalent; +mod mutable_keys; use std::hash::Hash; use std::hash::BuildHasher; @@ -18,8 +19,9 @@ use std::fmt; use std::mem::{replace}; use std::marker::PhantomData; -use util::{second, ptrdistance, enumerate}; +use util::{second, third, ptrdistance, enumerate}; pub use equivalent::Equivalent; +pub use mutable_keys::MutableKeys; fn hash_elem_using(build: &B, k: &K) -> HashValue { let mut h = build.build_hasher(); @@ -210,16 +212,6 @@ impl ShortHashProxy /// /// All iterators traverse the map in *the order*. /// -/// # Mutable Keys -/// -/// Some methods expose `&mut K`, mutable references to the key as it is stored -/// in the map. The key is allowed to be modified, but *only in a way that -/// preserves its hash and equality* (it is only useful for composite key -/// structs). -/// -/// This is sound (memory safe) but a logical error hazard (just like -/// implementing PartialEq, Eq, or Hash incorrectly would be). -/// /// # Examples /// /// ``` @@ -837,22 +829,11 @@ impl OrderMap pub fn get(&self, key: &Q) -> Option<&V> where Q: Hash + Equivalent, { - self.get_pair(key).map(second) - } - - pub fn get_pair(&self, key: &Q) -> Option<(&K, &V)> - where Q: Hash + Equivalent, - { - if let Some((_, found)) = self.find(key) { - let entry = &self.entries[found]; - Some((&entry.key, &entry.value)) - } else { - None - } + self.get_full(key).map(third) } /// Return item index, key and value - pub fn get_pair_index(&self, key: &Q) -> Option<(usize, &K, &V)> + pub fn get_full(&self, key: &Q) -> Option<(usize, &K, &V)> where Q: Hash + Equivalent, { if let Some((_, found)) = self.find(key) { @@ -866,31 +847,14 @@ impl OrderMap pub fn get_mut(&mut self, key: &Q) -> Option<&mut V> where Q: Hash + Equivalent, { - self.get_pair_mut(key).map(second) + self.get_full_mut(key).map(third) } - pub fn get_pair_mut(&mut self, key: &Q) - -> Option<(&mut K, &mut V)> + pub fn get_full_mut(&mut self, key: &Q) + -> Option<(usize, &K, &mut V)> where Q: Hash + Equivalent, { - if let Some((_, found)) = self.find(key) { - let entry = &mut self.entries[found]; - Some((&mut entry.key, &mut entry.value)) - } else { - None - } - } - - pub fn get_pair_index_mut(&mut self, key: &Q) - -> Option<(usize, &mut K, &mut V)> - where Q: Hash + Equivalent, - { - if let Some((_, found)) = self.find(key) { - let entry = &mut self.entries[found]; - Some((found, &mut entry.key, &mut entry.value)) - } else { - None - } + self.get_full_mut2(key).map(|(i, k, v)| (i, &*k, v)) } /// Return probe (indices) and position (entries) @@ -958,22 +922,9 @@ impl OrderMap /// /// Computes in **O(n)** time (average). pub fn retain(&mut self, mut keep: F) - where F: FnMut(&mut K, &mut V) -> bool, + where F: FnMut(&K, &mut V) -> bool, { - // We can use either forward or reverse scan, but forward was - // faster in a microbenchmark - let mut i = 0; - while i < self.len() { - { - let entry = &mut self.entries[i]; - if keep(&mut entry.key, &mut entry.value) { - i += 1; - continue; - } - } - self.swap_remove_index(i); - // skip increment on remove - } + self.retain2(move |k, v| keep(&*k, v)) } /// Sort the key-value pairs of the map and return a by value iterator of diff --git a/src/mutable_keys.rs b/src/mutable_keys.rs new file mode 100644 index 0000000000..acf73f4848 --- /dev/null +++ b/src/mutable_keys.rs @@ -0,0 +1,77 @@ + +use std::hash::Hash; +use std::hash::BuildHasher; + +use super::{OrderMap, Equivalent}; + +/// Opt-in mutable access to keys. +/// +/// You are allowed to modify the keys in the hashmap **if the modifcation +/// does not change the key's hash and equality**. +/// +/// If keys are modified erronously, you can no longer look them up. +/// +/// These methods expose `&mut K`, mutable references to the key as it is stored +/// in the map. The key is allowed to be modified, but *only in a way that +/// preserves its hash and equality* (it is only useful for composite key +/// structs). +/// +/// This is sound (memory safe) but a logical error hazard (just like +/// implementing PartialEq, Eq, or Hash incorrectly would be). +/// +pub trait MutableKeys { + type Key; + type Value; + + /// Return item index, mutable reference to key and value + fn get_full_mut2(&mut self, key: &Q) + -> Option<(usize, &mut Self::Key, &mut Self::Value)> + where Q: Hash + Equivalent; + + /// Scan through each key-value pair in the map and keep those where the + /// closure `keep` returns `true`. + /// + /// The order the elements are visited is not specified. + /// + /// Computes in **O(n)** time (average). + fn retain2(&mut self, keep: F) + where F: FnMut(&mut Self::Key, &mut Self::Value) -> bool; +} + +impl MutableKeys for OrderMap + where K: Eq + Hash, + S: BuildHasher, +{ + type Key = K; + type Value = V; + fn get_full_mut2(&mut self, key: &Q) + -> Option<(usize, &mut K, &mut V)> + where Q: Hash + Equivalent, + { + if let Some((_, found)) = self.find(key) { + let entry = &mut self.entries[found]; + Some((found, &mut entry.key, &mut entry.value)) + } else { + None + } + } + + fn retain2(&mut self, mut keep: F) + where F: FnMut(&mut K, &mut V) -> bool, + { + // We can use either forward or reverse scan, but forward was + // faster in a microbenchmark + let mut i = 0; + while i < self.len() { + { + let entry = &mut self.entries[i]; + if keep(&mut entry.key, &mut entry.value) { + i += 1; + continue; + } + } + self.swap_remove_index(i); + // skip increment on remove + } + } +} diff --git a/src/util.rs b/src/util.rs index 957426316a..ed120d7fea 100644 --- a/src/util.rs +++ b/src/util.rs @@ -3,6 +3,8 @@ use std::iter::Enumerate; use std::mem::size_of; pub fn second(t: (A, B)) -> B { t.1 } +pub fn third(t: (A, B, C)) -> C { t.2 } + pub fn enumerate(iterable: I) -> Enumerate where I: IntoIterator { From 32cd0e4415eb61f750724c1f4e69fa15ef3a3498 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 24 Sep 2017 20:25:00 +0200 Subject: [PATCH 2/2] FEAT: Rename swap_remove_pair to swap_remove_full Also return the index, hence the name full --- src/lib.rs | 35 +++++++++++++++++++---------------- src/util.rs | 1 - tests/quick.rs | 2 +- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 53fe7fd6f0..5a94c027b8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,7 @@ use std::fmt; use std::mem::{replace}; use std::marker::PhantomData; -use util::{second, third, ptrdistance, enumerate}; +use util::{third, ptrdistance, enumerate}; pub use equivalent::Equivalent; pub use mutable_keys::MutableKeys; @@ -866,6 +866,15 @@ impl OrderMap self.find_using(h, move |entry| { Q::equivalent(key, &entry.key) }) } + /// FIXME Same as .swap_remove + /// + /// Computes in **O(1)** time (average). + pub fn remove(&mut self, key: &Q) -> Option + where Q: Hash + Equivalent, + { + self.swap_remove(key) + } + /// Remove the key-value pair equivalent to `key` and return /// its value. /// @@ -879,33 +888,26 @@ impl OrderMap pub fn swap_remove(&mut self, key: &Q) -> Option where Q: Hash + Equivalent, { - self.swap_remove_pair(key).map(second) + self.swap_remove_full(key).map(third) } - /// FIXME Same as .swap_remove - /// - /// Computes in **O(1)** time (average). - pub fn remove(&mut self, key: &Q) -> Option - where Q: Hash + Equivalent, - { - self.swap_remove(key) - } - - /// Remove the key-value pair equivalent to `key` and return it. + /// Remove the key-value pair equivalent to `key` and return it and + /// the index it had. /// /// Like `Vec::swap_remove`, the pair is removed by swapping it with the /// last element of the map and popping it off. **This perturbs /// the postion of what used to be the last element!** /// /// Return `None` if `key` is not in map. - pub fn swap_remove_pair(&mut self, key: &Q) -> Option<(K, V)> + pub fn swap_remove_full(&mut self, key: &Q) -> Option<(usize, K, V)> where Q: Hash + Equivalent, { let (probe, found) = match self.find(key) { None => return None, Some(t) => t, }; - Some(self.remove_found(probe, found)) + let (k, v) = self.remove_found(probe, found); + Some((found, k, v)) } /// Remove the last key-value pair @@ -1663,12 +1665,13 @@ mod tests { let remove = [4, 12, 8, 7]; for &key in &remove_fail { - assert!(map.swap_remove_pair(&key).is_none()); + assert!(map.swap_remove_full(&key).is_none()); } println!("{:?}", map); for &key in &remove { //println!("{:?}", map); - assert_eq!(map.swap_remove_pair(&key), Some((key, key))); + let index = map.get_full(&key).unwrap().0; + assert_eq!(map.swap_remove_full(&key), Some((index, key, key))); } println!("{:?}", map); diff --git a/src/util.rs b/src/util.rs index ed120d7fea..90d3e7e334 100644 --- a/src/util.rs +++ b/src/util.rs @@ -2,7 +2,6 @@ use std::iter::Enumerate; use std::mem::size_of; -pub fn second(t: (A, B)) -> B { t.1 } pub fn third(t: (A, B, C)) -> C { t.2 } pub fn enumerate(iterable: I) -> Enumerate diff --git a/tests/quick.rs b/tests/quick.rs index 981929061d..b221a9520e 100644 --- a/tests/quick.rs +++ b/tests/quick.rs @@ -68,7 +68,7 @@ quickcheck! { map.insert(key, ()); } for &key in &remove { - map.swap_remove_pair(&key); + map.swap_remove(&key); } let elements = &set(&insert) - &set(&remove); map.len() == elements.len() && map.iter().count() == elements.len() &&