From d57ccf362414de0ee301ce74a6307d811e3885a3 Mon Sep 17 00:00:00 2001 From: The 8472 Date: Tue, 15 Nov 2022 22:02:00 +0100 Subject: [PATCH 1/2] Remove drain-on-drop behavior from DrainFilter --- src/map.rs | 41 +++++++---------------------------------- src/set.rs | 25 ++++++------------------- 2 files changed, 13 insertions(+), 53 deletions(-) diff --git a/src/map.rs b/src/map.rs index 6938801293..a537e82988 100644 --- a/src/map.rs +++ b/src/map.rs @@ -975,12 +975,9 @@ impl HashMap { /// Note that `drain_filter` lets you mutate every value in the filter closure, regardless of /// whether you choose to keep or remove it. /// - /// When the returned DrainedFilter is dropped, any remaining elements that satisfy - /// the predicate are dropped from the table. - /// - /// It is unspecified how many more elements will be subjected to the closure - /// if a panic occurs in the closure, or a panic occurs while dropping an element, - /// or if the `DrainFilter` value is leaked. + /// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating + /// or the iteration short-circuits, then the remaining elements will be retained. + /// Use [`retain()`] with a negated predicate if you do not need the returned iterator. /// /// Keeps the allocated memory for reuse. /// @@ -1007,9 +1004,8 @@ impl HashMap { /// let d = map.drain_filter(|k, _v| k % 2 != 0); /// } /// - /// // But the map lens have been reduced by half - /// // even if we do not use DrainFilter iterator. - /// assert_eq!(map.len(), 4); + /// // DrainFilter was not exhausted, therefore no elements were drained. + /// assert_eq!(map.len(), 8); /// ``` #[cfg_attr(feature = "inline-more", inline)] pub fn drain_filter(&mut self, f: F) -> DrainFilter<'_, K, V, F, A> @@ -2760,6 +2756,7 @@ impl Drain<'_, K, V, A> { /// /// assert_eq!(map.len(), 1); /// ``` +#[must_use = "Iterators are lazy unless consumed"] pub struct DrainFilter<'a, K, V, F, A: Allocator + Clone = Global> where F: FnMut(&K, &mut V) -> bool, @@ -2768,30 +2765,6 @@ where inner: DrainFilterInner<'a, K, V, A>, } -impl<'a, K, V, F, A> Drop for DrainFilter<'a, K, V, F, A> -where - F: FnMut(&K, &mut V) -> bool, - A: Allocator + Clone, -{ - #[cfg_attr(feature = "inline-more", inline)] - fn drop(&mut self) { - while let Some(item) = self.next() { - let guard = ConsumeAllOnDrop(self); - drop(item); - mem::forget(guard); - } - } -} - -pub(super) struct ConsumeAllOnDrop<'a, T: Iterator>(pub &'a mut T); - -impl Drop for ConsumeAllOnDrop<'_, T> { - #[cfg_attr(feature = "inline-more", inline)] - fn drop(&mut self) { - self.0.for_each(drop); - } -} - impl Iterator for DrainFilter<'_, K, V, F, A> where F: FnMut(&K, &mut V) -> bool, @@ -8180,7 +8153,7 @@ mod test_map { } { let mut map: HashMap = (0..8).map(|x| (x, x * 10)).collect(); - drop(map.drain_filter(|&k, _| k % 2 == 0)); + map.drain_filter(|&k, _| k % 2 == 0).for_each(drop); assert_eq!(map.len(), 4); } } diff --git a/src/set.rs b/src/set.rs index f5670d160d..e615877f06 100644 --- a/src/set.rs +++ b/src/set.rs @@ -5,10 +5,9 @@ use alloc::borrow::ToOwned; use core::fmt; use core::hash::{BuildHasher, Hash}; use core::iter::{Chain, FromIterator, FusedIterator}; -use core::mem; use core::ops::{BitAnd, BitOr, BitXor, Sub}; -use super::map::{self, ConsumeAllOnDrop, DefaultHashBuilder, DrainFilterInner, HashMap, Keys}; +use super::map::{self, DefaultHashBuilder, DrainFilterInner, HashMap, Keys}; use crate::raw::{Allocator, Global}; // Future Optimization (FIXME!) @@ -380,8 +379,9 @@ impl HashSet { /// In other words, move all elements `e` such that `f(&e)` returns `true` out /// into another iterator. /// - /// When the returned DrainedFilter is dropped, any remaining elements that satisfy - /// the predicate are dropped from the set. + /// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating + /// or the iteration short-circuits, then the remaining elements will be retained. + /// Use [`retain()`] with a negated predicate if you do not need the returned iterator. /// /// # Examples /// @@ -1572,6 +1572,7 @@ pub struct Drain<'a, K, A: Allocator + Clone = Global> { /// /// [`drain_filter`]: struct.HashSet.html#method.drain_filter /// [`HashSet`]: struct.HashSet.html +#[must_use = "Iterators are lazy unless consumed"] pub struct DrainFilter<'a, K, F, A: Allocator + Clone = Global> where F: FnMut(&K) -> bool, @@ -1768,20 +1769,6 @@ impl fmt::Debug for Drain<'_, K, A> { } } -impl<'a, K, F, A: Allocator + Clone> Drop for DrainFilter<'a, K, F, A> -where - F: FnMut(&K) -> bool, -{ - #[cfg_attr(feature = "inline-more", inline)] - fn drop(&mut self) { - while let Some(item) = self.next() { - let guard = ConsumeAllOnDrop(self); - drop(item); - mem::forget(guard); - } - } -} - impl Iterator for DrainFilter<'_, K, F, A> where F: FnMut(&K) -> bool, @@ -2861,7 +2848,7 @@ mod test_set { } { let mut set: HashSet = (0..8).collect(); - drop(set.drain_filter(|&k| k % 2 == 0)); + set.drain_filter(|&k| k % 2 == 0).for_each(drop); assert_eq!(set.len(), 4, "Removes non-matching items on drop"); } } From a0ab1dea3dc6926b82702d59edd92f90f505084a Mon Sep 17 00:00:00 2001 From: The 8472 Date: Mon, 10 Apr 2023 14:17:52 +0200 Subject: [PATCH 2/2] Rename drain_filter to extract_if --- src/map.rs | 52 ++++++++++++++++++++++++++-------------------------- src/set.rs | 33 +++++++++++++++------------------ 2 files changed, 41 insertions(+), 44 deletions(-) diff --git a/src/map.rs b/src/map.rs index a537e82988..8e88838b11 100644 --- a/src/map.rs +++ b/src/map.rs @@ -972,10 +972,10 @@ impl HashMap { /// In other words, move all pairs `(k, v)` such that `f(&k, &mut v)` returns `true` out /// into another iterator. /// - /// Note that `drain_filter` lets you mutate every value in the filter closure, regardless of + /// Note that `extract_if` lets you mutate every value in the filter closure, regardless of /// whether you choose to keep or remove it. /// - /// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating + /// If the returned `ExtractIf` is not exhausted, e.g. because it is dropped without iterating /// or the iteration short-circuits, then the remaining elements will be retained. /// Use [`retain()`] with a negated predicate if you do not need the returned iterator. /// @@ -988,7 +988,7 @@ impl HashMap { /// /// let mut map: HashMap = (0..8).map(|x| (x, x)).collect(); /// - /// let drained: HashMap = map.drain_filter(|k, _v| k % 2 == 0).collect(); + /// let drained: HashMap = map.extract_if(|k, _v| k % 2 == 0).collect(); /// /// let mut evens = drained.keys().cloned().collect::>(); /// let mut odds = map.keys().cloned().collect::>(); @@ -1001,20 +1001,20 @@ impl HashMap { /// let mut map: HashMap = (0..8).map(|x| (x, x)).collect(); /// /// { // Iterator is dropped without being consumed. - /// let d = map.drain_filter(|k, _v| k % 2 != 0); + /// let d = map.extract_if(|k, _v| k % 2 != 0); /// } /// - /// // DrainFilter was not exhausted, therefore no elements were drained. + /// // ExtractIf was not exhausted, therefore no elements were drained. /// assert_eq!(map.len(), 8); /// ``` #[cfg_attr(feature = "inline-more", inline)] - pub fn drain_filter(&mut self, f: F) -> DrainFilter<'_, K, V, F, A> + pub fn extract_if(&mut self, f: F) -> ExtractIf<'_, K, V, F, A> where F: FnMut(&K, &mut V) -> bool, { - DrainFilter { + ExtractIf { f, - inner: DrainFilterInner { + inner: ExtractIfInner { iter: unsafe { self.table.iter() }, table: &mut self.table, }, @@ -2728,10 +2728,10 @@ impl Drain<'_, K, V, A> { /// A draining iterator over entries of a `HashMap` which don't satisfy the predicate /// `f(&k, &mut v)` in arbitrary order. The iterator element type is `(K, V)`. /// -/// This `struct` is created by the [`drain_filter`] method on [`HashMap`]. See its +/// This `struct` is created by the [`extract_if`] method on [`HashMap`]. See its /// documentation for more. /// -/// [`drain_filter`]: struct.HashMap.html#method.drain_filter +/// [`extract_if`]: struct.HashMap.html#method.extract_if /// [`HashMap`]: struct.HashMap.html /// /// # Examples @@ -2741,31 +2741,31 @@ impl Drain<'_, K, V, A> { /// /// let mut map: HashMap = [(1, "a"), (2, "b"), (3, "c")].into(); /// -/// let mut drain_filter = map.drain_filter(|k, _v| k % 2 != 0); -/// let mut vec = vec![drain_filter.next(), drain_filter.next()]; +/// let mut extract_if = map.extract_if(|k, _v| k % 2 != 0); +/// let mut vec = vec![extract_if.next(), extract_if.next()]; /// -/// // The `DrainFilter` iterator produces items in arbitrary order, so the +/// // The `ExtractIf` iterator produces items in arbitrary order, so the /// // items must be sorted to test them against a sorted array. /// vec.sort_unstable(); /// assert_eq!(vec, [Some((1, "a")),Some((3, "c"))]); /// /// // It is fused iterator -/// assert_eq!(drain_filter.next(), None); -/// assert_eq!(drain_filter.next(), None); -/// drop(drain_filter); +/// assert_eq!(extract_if.next(), None); +/// assert_eq!(extract_if.next(), None); +/// drop(extract_if); /// /// assert_eq!(map.len(), 1); /// ``` #[must_use = "Iterators are lazy unless consumed"] -pub struct DrainFilter<'a, K, V, F, A: Allocator + Clone = Global> +pub struct ExtractIf<'a, K, V, F, A: Allocator + Clone = Global> where F: FnMut(&K, &mut V) -> bool, { f: F, - inner: DrainFilterInner<'a, K, V, A>, + inner: ExtractIfInner<'a, K, V, A>, } -impl Iterator for DrainFilter<'_, K, V, F, A> +impl Iterator for ExtractIf<'_, K, V, F, A> where F: FnMut(&K, &mut V) -> bool, A: Allocator + Clone, @@ -2783,15 +2783,15 @@ where } } -impl FusedIterator for DrainFilter<'_, K, V, F> where F: FnMut(&K, &mut V) -> bool {} +impl FusedIterator for ExtractIf<'_, K, V, F> where F: FnMut(&K, &mut V) -> bool {} -/// Portions of `DrainFilter` shared with `set::DrainFilter` -pub(super) struct DrainFilterInner<'a, K, V, A: Allocator + Clone> { +/// Portions of `ExtractIf` shared with `set::ExtractIf` +pub(super) struct ExtractIfInner<'a, K, V, A: Allocator + Clone> { pub iter: RawIter<(K, V)>, pub table: &'a mut RawTable<(K, V), A>, } -impl DrainFilterInner<'_, K, V, A> { +impl ExtractIfInner<'_, K, V, A> { #[cfg_attr(feature = "inline-more", inline)] pub(super) fn next(&mut self, f: &mut F) -> Option<(K, V)> where @@ -8142,10 +8142,10 @@ mod test_map { } #[test] - fn test_drain_filter() { + fn test_extract_if() { { let mut map: HashMap = (0..8).map(|x| (x, x * 10)).collect(); - let drained = map.drain_filter(|&k, _| k % 2 == 0); + let drained = map.extract_if(|&k, _| k % 2 == 0); let mut out = drained.collect::>(); out.sort_unstable(); assert_eq!(vec![(0, 0), (2, 20), (4, 40), (6, 60)], out); @@ -8153,7 +8153,7 @@ mod test_map { } { let mut map: HashMap = (0..8).map(|x| (x, x * 10)).collect(); - map.drain_filter(|&k, _| k % 2 == 0).for_each(drop); + map.extract_if(|&k, _| k % 2 == 0).for_each(drop); assert_eq!(map.len(), 4); } } diff --git a/src/set.rs b/src/set.rs index e615877f06..4486f7c820 100644 --- a/src/set.rs +++ b/src/set.rs @@ -7,7 +7,7 @@ use core::hash::{BuildHasher, Hash}; use core::iter::{Chain, FromIterator, FusedIterator}; use core::ops::{BitAnd, BitOr, BitXor, Sub}; -use super::map::{self, DefaultHashBuilder, DrainFilterInner, HashMap, Keys}; +use super::map::{self, DefaultHashBuilder, ExtractIfInner, HashMap, Keys}; use crate::raw::{Allocator, Global}; // Future Optimization (FIXME!) @@ -379,7 +379,7 @@ impl HashSet { /// In other words, move all elements `e` such that `f(&e)` returns `true` out /// into another iterator. /// - /// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating + /// If the returned `ExtractIf` is not exhausted, e.g. because it is dropped without iterating /// or the iteration short-circuits, then the remaining elements will be retained. /// Use [`retain()`] with a negated predicate if you do not need the returned iterator. /// @@ -389,7 +389,7 @@ impl HashSet { /// use hashbrown::HashSet; /// /// let mut set: HashSet = (0..8).collect(); - /// let drained: HashSet = set.drain_filter(|v| v % 2 == 0).collect(); + /// let drained: HashSet = set.extract_if(|v| v % 2 == 0).collect(); /// /// let mut evens = drained.into_iter().collect::>(); /// let mut odds = set.into_iter().collect::>(); @@ -400,13 +400,13 @@ impl HashSet { /// assert_eq!(odds, vec![1, 3, 5, 7]); /// ``` #[cfg_attr(feature = "inline-more", inline)] - pub fn drain_filter(&mut self, f: F) -> DrainFilter<'_, T, F, A> + pub fn extract_if(&mut self, f: F) -> ExtractIf<'_, T, F, A> where F: FnMut(&T) -> bool, { - DrainFilter { + ExtractIf { f, - inner: DrainFilterInner { + inner: ExtractIfInner { iter: unsafe { self.map.table.iter() }, table: &mut self.map.table, }, @@ -1567,18 +1567,18 @@ pub struct Drain<'a, K, A: Allocator + Clone = Global> { /// A draining iterator over entries of a `HashSet` which don't satisfy the predicate `f`. /// -/// This `struct` is created by the [`drain_filter`] method on [`HashSet`]. See its +/// This `struct` is created by the [`extract_if`] method on [`HashSet`]. See its /// documentation for more. /// -/// [`drain_filter`]: struct.HashSet.html#method.drain_filter +/// [`extract_if`]: struct.HashSet.html#method.extract_if /// [`HashSet`]: struct.HashSet.html #[must_use = "Iterators are lazy unless consumed"] -pub struct DrainFilter<'a, K, F, A: Allocator + Clone = Global> +pub struct ExtractIf<'a, K, F, A: Allocator + Clone = Global> where F: FnMut(&K) -> bool, { f: F, - inner: DrainFilterInner<'a, K, (), A>, + inner: ExtractIfInner<'a, K, (), A>, } /// A lazy iterator producing elements in the intersection of `HashSet`s. @@ -1769,7 +1769,7 @@ impl fmt::Debug for Drain<'_, K, A> { } } -impl Iterator for DrainFilter<'_, K, F, A> +impl Iterator for ExtractIf<'_, K, F, A> where F: FnMut(&K) -> bool, { @@ -1788,10 +1788,7 @@ where } } -impl FusedIterator for DrainFilter<'_, K, F, A> where - F: FnMut(&K) -> bool -{ -} +impl FusedIterator for ExtractIf<'_, K, F, A> where F: FnMut(&K) -> bool {} impl Clone for Intersection<'_, T, S, A> { #[cfg_attr(feature = "inline-more", inline)] @@ -2837,10 +2834,10 @@ mod test_set { } #[test] - fn test_drain_filter() { + fn test_extract_if() { { let mut set: HashSet = (0..8).collect(); - let drained = set.drain_filter(|&k| k % 2 == 0); + let drained = set.extract_if(|&k| k % 2 == 0); let mut out = drained.collect::>(); out.sort_unstable(); assert_eq!(vec![0, 2, 4, 6], out); @@ -2848,7 +2845,7 @@ mod test_set { } { let mut set: HashSet = (0..8).collect(); - set.drain_filter(|&k| k % 2 == 0).for_each(drop); + set.extract_if(|&k| k % 2 == 0).for_each(drop); assert_eq!(set.len(), 4, "Removes non-matching items on drop"); } }