From 8265ca091b6d8928b6efd3a05a5b4459e88ca3e2 Mon Sep 17 00:00:00 2001 From: Carl Lerche Date: Mon, 8 May 2017 13:10:07 -0700 Subject: [PATCH] Align insert & remove HeaderMap APIs with HashMap (#19) This PR switches insert and remove to return `Option` instead of a drain iterator. This matches the API provided by HashMap. Additional APIs are provided on OccupiedEntry to insert / remove the entry while returning all previously associated values. Also removes `remove_entry` from HeaderMap. --- src/header/map.rs | 219 ++++++++++++++++++++++++++++---------------- tests/header_map.rs | 7 +- 2 files changed, 148 insertions(+), 78 deletions(-) diff --git a/src/header/map.rs b/src/header/map.rs index 7724354..2810f0b 100644 --- a/src/header/map.rs +++ b/src/header/map.rs @@ -1005,9 +1005,14 @@ impl HeaderMap { /// returned. /// /// If the map did have this key present, the new value is associated with - /// the key and all previous values are removed and returned. The key is not - /// updated, though; this matters for types that can be `==` without being - /// identical. + /// the key and all previous values are removed. **Note** that only a single + /// one of the previous values is returned. If there are multiple values + /// that have been previously associated with the key, then the first one is + /// returned. See `insert_mult` on `OccupiedEntry` for an API that returns + /// all values. + /// + /// The key is not updated, though; this matters for types that can be `==` + /// without being identical. /// /// # Examples /// @@ -1018,17 +1023,16 @@ impl HeaderMap { /// assert!(!map.is_empty()); /// /// let mut prev = map.insert("x-hello", "earth").unwrap(); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); + /// assert_eq!("world", prev); /// ``` - pub fn insert(&mut self, key: K, val: T) -> Option> + pub fn insert(&mut self, key: K, val: T) -> Option where K: HeaderMapKey, { key.insert(self, val.into()) } #[inline] - fn insert2(&mut self, key: K, value: T) -> Option> + fn insert2(&mut self, key: K, value: T) -> Option where K: Hash + Into, HeaderName: PartialEq, { @@ -1060,8 +1064,25 @@ impl HeaderMap { /// Set an occupied bucket to the given value #[inline] - fn insert_occupied(&mut self, index: usize, value: T) -> DrainEntry { - // TODO: Looks like this is repeated code + fn insert_occupied(&mut self, index: usize, value: T) -> T { + let old; + let links; + + { + let entry = &mut self.entries[index as usize]; + + old = mem::replace(&mut entry.value, value); + links = entry.links.take(); + } + + if let Some(links) = links { + self.remove_all_extra_values(links.next); + } + + old + } + + fn insert_occupied_mult(&mut self, index: usize, value: T) -> DrainEntry { let old; let links; @@ -1204,10 +1225,12 @@ impl HeaderMap { index } - /// Removes a key from the map, returning the values associated with the - /// key. + /// Removes a key from the map, returning the value associated with the key. /// - /// Returns `None` if the map does not contain the key. + /// Returns `None` if the map does not contain the key. If there are + /// multiple values associated with the key, then the first one is returned. + /// See `remove_entry_mult` on `OccupiedEntry` for an API that yields all + /// values. /// /// # Examples /// @@ -1216,47 +1239,23 @@ impl HeaderMap { /// let mut map = HeaderMap::new(); /// map.insert("x-hello", "world"); /// - /// { - /// let mut prev = map.remove("x-hello").unwrap(); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); - /// } + /// let mut prev = map.remove("x-hello").unwrap(); + /// assert_eq!("world", prev); /// /// assert!(map.remove("x-hello").is_none()); /// ``` - pub fn remove(&mut self, key: &K) -> Option> - where K: HeaderMapKey - { - self.remove_entry(key).map(|e| e.1) - } - - /// Removes a key from the map, returning the key and all values associated - /// with the key. - /// - /// Returns `None` if the map does not contain the key. - /// - /// # Examples - /// - /// ``` - /// # use http::HeaderMap; - /// let mut map = HeaderMap::new(); - /// map.insert("x-hello", "world"); - /// - /// { - /// let (key, mut prev) = map.remove_entry("x-hello").unwrap(); - /// assert_eq!("x-hello", key.as_str()); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); - /// } - /// - /// assert!(map.remove("x-hello").is_none()); - /// ``` - pub fn remove_entry(&mut self, key: &K) -> Option<(HeaderName, DrainEntry)> + pub fn remove(&mut self, key: &K) -> Option where K: HeaderMapKey { match key.find(self) { Some((probe, idx)) => { - Some(self.remove_found(probe, idx)) + let entry = self.remove_found(probe, idx); + + if let Some(links) = entry.links { + self.remove_all_extra_values(links.next); + } + + Some(entry.value) } None => None, } @@ -1264,10 +1263,7 @@ impl HeaderMap { /// Remove an entry from the map while in hashed mode #[inline] - fn remove_found(&mut self, - probe: usize, - found: usize) -> (HeaderName, DrainEntry) - { + fn remove_found(&mut self, probe: usize, found: usize) -> Bucket { // index `probe` and entry `found` is to be removed // use swap_remove, but then we need to update the index that points // to the other entry that has to move @@ -1313,14 +1309,7 @@ impl HeaderMap { }); } - let drain = DrainEntry { - map: self as *mut _, - first: Some(entry.value), - next: entry.links.map(|l| l.next), - lt: PhantomData, - }; - - (entry.key, drain) + entry } /// Removes the `ExtraValue` at the given index. @@ -1427,6 +1416,18 @@ impl HeaderMap { extra } + fn remove_all_extra_values(&mut self, mut head: usize) { + loop { + let extra = self.remove_extra_value(head); + + if let Link::Extra(idx) = extra.next { + head = idx; + } else { + break; + } + } + } + #[inline] fn insert_entry(&mut self, hash: HashValue, key: HeaderName, value: T) { assert!(self.entries.len() < MAX_SIZE, "header map at capacity"); @@ -2387,7 +2388,8 @@ impl<'a, T> OccupiedEntry<'a, T> { /// Sets the value of the entry. /// - /// All previous values associated with the entry are removed and returned. + /// All previous values associated with the entry are removed and the first + /// one is returned. See `insert_mult` for an API that returns all values. /// /// # Examples /// @@ -2398,17 +2400,42 @@ impl<'a, T> OccupiedEntry<'a, T> { /// /// if let Entry::Occupied(mut e) = map.entry("x-hello") { /// let mut prev = e.insert("earth"); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); + /// assert_eq!("world", prev); /// } /// /// assert_eq!("earth", map["x-hello"]); /// ``` #[inline] - pub fn insert(&mut self, value: T) -> DrainEntry { + pub fn insert(&mut self, value: T) -> T { self.map.insert_occupied(self.index, value.into()) } + /// Sets the value of the entry. + /// + /// This function does the same as `insert` except it returns an iterator + /// that yields all values previously associated with the key. + /// + /// # Examples + /// + /// ``` + /// # use http::header::{HeaderMap, Entry}; + /// let mut map = HeaderMap::new(); + /// map.insert("x-hello", "world"); + /// map.append("x-hello", "world2"); + /// + /// if let Entry::Occupied(mut e) = map.entry("x-hello") { + /// let mut prev = e.insert_mult("earth"); + /// assert_eq!("world", prev.next().unwrap()); + /// assert_eq!("world2", prev.next().unwrap()); + /// assert!(prev.next().is_none()); + /// } + /// + /// assert_eq!("earth", map["x-hello"]); + /// ``` + pub fn insert_mult(&mut self, value: T) -> DrainEntry { + self.map.insert_occupied_mult(self.index, value.into()) + } + /// Insert the value into the entry. /// /// The new value is appended to the end of the entry's value list. All @@ -2438,7 +2465,8 @@ impl<'a, T> OccupiedEntry<'a, T> { /// Remove the entry from the map. /// - /// Any values currently stored in the entry are returned. + /// All values associated with the entry are removed and the first one is + /// returned. See `remove_entry_mult` for an API that returns all values. /// /// # Examples /// @@ -2449,19 +2477,20 @@ impl<'a, T> OccupiedEntry<'a, T> { /// /// if let Entry::Occupied(e) = map.entry("x-hello") { /// let mut prev = e.remove(); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); + /// assert_eq!("world", prev); /// } /// /// assert!(!map.contains_key("x-hello")); /// ``` - pub fn remove(self) -> DrainEntry<'a, T> { + pub fn remove(self) -> T { self.remove_entry().1 } /// Remove the entry from the map. /// - /// They key and any values currently stored in the entry are returned. + /// The key and all values associated with the entry are removed and the + /// first one is returned. See `remove_entry_mult` for an API that returns + /// all values. /// /// # Examples /// @@ -2473,14 +2502,50 @@ impl<'a, T> OccupiedEntry<'a, T> { /// if let Entry::Occupied(e) = map.entry("x-hello") { /// let (key, mut prev) = e.remove_entry(); /// assert_eq!("x-hello", key.as_str()); - /// assert_eq!("world", prev.next().unwrap()); - /// assert!(prev.next().is_none()); + /// assert_eq!("world", prev); /// } /// /// assert!(!map.contains_key("x-hello")); /// ``` - pub fn remove_entry(self) -> (HeaderName, DrainEntry<'a, T>) { - self.map.remove_found(self.probe, self.index) + pub fn remove_entry(self) -> (HeaderName, T) { + let entry = self.map.remove_found(self.probe, self.index); + + if let Some(links) = entry.links { + self.map.remove_all_extra_values(links.next); + } + + (entry.key, entry.value) + } + + /// Remove the entry from the map. + /// + /// The key and all values associated with the entry are removed and + /// returned. See `remove_entry_mult` for an API that returns all values. + /// + /// # Examples + /// + /// ``` + /// # use http::header::{HeaderMap, Entry}; + /// let mut map = HeaderMap::new(); + /// map.insert("x-hello", "world"); + /// + /// if let Entry::Occupied(e) = map.entry("x-hello") { + /// let (key, mut prev) = e.remove_entry(); + /// assert_eq!("x-hello", key.as_str()); + /// assert_eq!("world", prev); + /// } + /// + /// assert!(!map.contains_key("x-hello")); + /// ``` + pub fn remove_entry_mult(self) -> (HeaderName, DrainEntry<'a, T>) { + let entry = self.map.remove_found(self.probe, self.index); + let drain = DrainEntry { + map: self.map as *mut _, + first: Some(entry.value), + next: entry.links.map(|l| l.next), + lt: PhantomData, + }; + (entry.key, drain) } /// Returns an iterator visiting all values associated with the entry. @@ -2749,7 +2814,7 @@ pub trait HeaderMapKey: Sealed { // cases where DST values can be passed in. #[doc(hidden)] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> + fn insert(self, map: &mut HeaderMap, val: T) -> Option where Self: Sized { drop(map); @@ -2785,7 +2850,7 @@ pub trait Sealed {} impl HeaderMapKey for HeaderName { #[doc(hidden)] #[inline] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> { + fn insert(self, map: &mut HeaderMap, val: T) -> Option { map.insert2(self, val) } @@ -2819,7 +2884,7 @@ impl Sealed for HeaderName {} impl<'a> HeaderMapKey for &'a HeaderName { #[doc(hidden)] #[inline] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> { + fn insert(self, map: &mut HeaderMap, val: T) -> Option { map.insert2(self, val) } @@ -2869,7 +2934,7 @@ impl Sealed for str {} impl<'a> HeaderMapKey for &'a str { #[doc(hidden)] #[inline] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> { + fn insert(self, map: &mut HeaderMap, val: T) -> Option { HdrName::from_bytes(self.as_bytes(), move |hdr| map.insert2(hdr, val)).unwrap() } @@ -2903,7 +2968,7 @@ impl<'a> Sealed for &'a str {} impl HeaderMapKey for String { #[doc(hidden)] #[inline] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> { + fn insert(self, map: &mut HeaderMap, val: T) -> Option { self.as_str().insert(map, val) } @@ -2937,7 +3002,7 @@ impl Sealed for String {} impl<'a> HeaderMapKey for &'a String { #[doc(hidden)] #[inline] - fn insert(self, map: &mut HeaderMap, val: T) -> Option> { + fn insert(self, map: &mut HeaderMap, val: T) -> Option { self.as_str().insert(map, val) } diff --git a/tests/header_map.rs b/tests/header_map.rs index b2fca5b..c878dcb 100644 --- a/tests/header_map.rs +++ b/tests/header_map.rs @@ -97,7 +97,12 @@ fn drain_entry() { // Using insert { - let vals: Vec<_> = headers.insert("hello", "wat").unwrap().collect(); + let mut e = match headers.entry("hello") { + Entry::Occupied(e) => e, + _ => panic!(), + }; + + let vals: Vec<_> = e.insert_mult("wat").collect(); assert_eq!(2, vals.len()); assert_eq!(vals[0], "world"); assert_eq!(vals[1], "world2");