Skip to content

Commit b687e84

Browse files
the8472the8472
committed
remove drain-on-drop behavior from BTree{Set,Map}::DrainFilter and add #[must_use]
1 parent c0df1c8 commit b687e84

File tree

4 files changed

+40
-58
lines changed

4 files changed

+40
-58
lines changed

‎library/alloc/src/collections/btree/map.rs‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,7 +1132,7 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
11321132
K: Ord,
11331133
F: FnMut(&K, &mut V) -> bool,
11341134
{
1135-
self.drain_filter(|k, v| !f(k, v));
1135+
self.drain_filter(|k, v| !f(k, v)).for_each(drop);
11361136
}
11371137

11381138
/// Moves all elements from `other` into `self`, leaving `other` empty.
@@ -1395,14 +1395,11 @@ impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> {
13951395
/// The iterator also lets you mutate the value of each element in the
13961396
/// closure, regardless of whether you choose to keep or remove it.
13971397
///
1398-
/// If the iterator is only partially consumed or not consumed at all, each
1399-
/// of the remaining elements is still subjected to the closure, which may
1400-
/// change its value and, by returning `true`, have the element removed and
1401-
/// dropped.
1398+
/// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating
1399+
/// or the iteration short-circuits, then the remaining elements will be retained.
1400+
/// Use [`retain`] with a negated predicate if you do not need the returned iterator.
14021401
///
1403-
/// It is unspecified how many more elements will be subjected to the
1404-
/// closure if a panic occurs in the closure, or a panic occurs while
1405-
/// dropping an element, or if the `DrainFilter` value is leaked.
1402+
/// [`retain`]: BTreeMap::retain
14061403
///
14071404
/// # Examples
14081405
///
@@ -1901,6 +1898,7 @@ impl<K, V> Default for Values<'_, K, V> {
19011898

19021899
/// An iterator produced by calling `drain_filter` on BTreeMap.
19031900
#[unstable(feature = "btree_drain_filter", issue = "70530")]
1901+
#[must_use = "iterators are lazy and do nothing unless consumed"]
19041902
pub struct DrainFilter<
19051903
'a,
19061904
K,
@@ -1929,16 +1927,6 @@ pub(super) struct DrainFilterInner<'a, K, V> {
19291927
cur_leaf_edge: Option<Handle<NodeRef<marker::Mut<'a>, K, V, marker::Leaf>, marker::Edge>>,
19301928
}
19311929

1932-
#[unstable(feature = "btree_drain_filter", issue = "70530")]
1933-
impl<K, V, F, A: Allocator + Clone> Drop for DrainFilter<'_, K, V, F, A>
1934-
where
1935-
F: FnMut(&K, &mut V) -> bool,
1936-
{
1937-
fn drop(&mut self) {
1938-
self.for_each(drop);
1939-
}
1940-
}
1941-
19421930
#[unstable(feature = "btree_drain_filter", issue = "70530")]
19431931
impl<K, V, F> fmt::Debug for DrainFilter<'_, K, V, F>
19441932
where

‎library/alloc/src/collections/btree/map/tests.rs‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -947,7 +947,7 @@ mod test_drain_filter {
947947
#[test]
948948
fn empty() {
949949
let mut map: BTreeMap<i32, i32> = BTreeMap::new();
950-
map.drain_filter(|_, _| unreachable!("there's nothing to decide on"));
950+
map.drain_filter(|_, _| unreachable!("there's nothing to decide on")).for_each(drop);
951951
assert_eq!(map.height(), None);
952952
map.check();
953953
}
@@ -1008,7 +1008,7 @@ mod test_drain_filter {
10081008
fn underfull_keeping_all() {
10091009
let pairs = (0..3).map(|i| (i, i));
10101010
let mut map = BTreeMap::from_iter(pairs);
1011-
map.drain_filter(|_, _| false);
1011+
map.drain_filter(|_, _| false).for_each(drop);
10121012
assert!(map.keys().copied().eq(0..3));
10131013
map.check();
10141014
}
@@ -1018,7 +1018,7 @@ mod test_drain_filter {
10181018
let pairs = (0..3).map(|i| (i, i));
10191019
for doomed in 0..3 {
10201020
let mut map = BTreeMap::from_iter(pairs.clone());
1021-
map.drain_filter(|i, _| *i == doomed);
1021+
map.drain_filter(|i, _| *i == doomed).for_each(drop);
10221022
assert_eq!(map.len(), 2);
10231023
map.check();
10241024
}
@@ -1029,7 +1029,7 @@ mod test_drain_filter {
10291029
let pairs = (0..3).map(|i| (i, i));
10301030
for sacred in 0..3 {
10311031
let mut map = BTreeMap::from_iter(pairs.clone());
1032-
map.drain_filter(|i, _| *i != sacred);
1032+
map.drain_filter(|i, _| *i != sacred).for_each(drop);
10331033
assert!(map.keys().copied().eq(sacred..=sacred));
10341034
map.check();
10351035
}
@@ -1039,7 +1039,7 @@ mod test_drain_filter {
10391039
fn underfull_removing_all() {
10401040
let pairs = (0..3).map(|i| (i, i));
10411041
let mut map = BTreeMap::from_iter(pairs);
1042-
map.drain_filter(|_, _| true);
1042+
map.drain_filter(|_, _| true).for_each(drop);
10431043
assert!(map.is_empty());
10441044
map.check();
10451045
}
@@ -1048,7 +1048,7 @@ mod test_drain_filter {
10481048
fn height_0_keeping_all() {
10491049
let pairs = (0..node::CAPACITY).map(|i| (i, i));
10501050
let mut map = BTreeMap::from_iter(pairs);
1051-
map.drain_filter(|_, _| false);
1051+
map.drain_filter(|_, _| false).for_each(drop);
10521052
assert!(map.keys().copied().eq(0..node::CAPACITY));
10531053
map.check();
10541054
}
@@ -1058,7 +1058,7 @@ mod test_drain_filter {
10581058
let pairs = (0..node::CAPACITY).map(|i| (i, i));
10591059
for doomed in 0..node::CAPACITY {
10601060
let mut map = BTreeMap::from_iter(pairs.clone());
1061-
map.drain_filter(|i, _| *i == doomed);
1061+
map.drain_filter(|i, _| *i == doomed).for_each(drop);
10621062
assert_eq!(map.len(), node::CAPACITY - 1);
10631063
map.check();
10641064
}
@@ -1069,7 +1069,7 @@ mod test_drain_filter {
10691069
let pairs = (0..node::CAPACITY).map(|i| (i, i));
10701070
for sacred in 0..node::CAPACITY {
10711071
let mut map = BTreeMap::from_iter(pairs.clone());
1072-
map.drain_filter(|i, _| *i != sacred);
1072+
map.drain_filter(|i, _| *i != sacred).for_each(drop);
10731073
assert!(map.keys().copied().eq(sacred..=sacred));
10741074
map.check();
10751075
}
@@ -1079,7 +1079,7 @@ mod test_drain_filter {
10791079
fn height_0_removing_all() {
10801080
let pairs = (0..node::CAPACITY).map(|i| (i, i));
10811081
let mut map = BTreeMap::from_iter(pairs);
1082-
map.drain_filter(|_, _| true);
1082+
map.drain_filter(|_, _| true).for_each(drop);
10831083
assert!(map.is_empty());
10841084
map.check();
10851085
}
@@ -1096,7 +1096,7 @@ mod test_drain_filter {
10961096
fn height_1_removing_all() {
10971097
let pairs = (0..MIN_INSERTS_HEIGHT_1).map(|i| (i, i));
10981098
let mut map = BTreeMap::from_iter(pairs);
1099-
map.drain_filter(|_, _| true);
1099+
map.drain_filter(|_, _| true).for_each(drop);
11001100
assert!(map.is_empty());
11011101
map.check();
11021102
}
@@ -1106,7 +1106,7 @@ mod test_drain_filter {
11061106
let pairs = (0..MIN_INSERTS_HEIGHT_1).map(|i| (i, i));
11071107
for doomed in 0..MIN_INSERTS_HEIGHT_1 {
11081108
let mut map = BTreeMap::from_iter(pairs.clone());
1109-
map.drain_filter(|i, _| *i == doomed);
1109+
map.drain_filter(|i, _| *i == doomed).for_each(drop);
11101110
assert_eq!(map.len(), MIN_INSERTS_HEIGHT_1 - 1);
11111111
map.check();
11121112
}
@@ -1117,7 +1117,7 @@ mod test_drain_filter {
11171117
let pairs = (0..MIN_INSERTS_HEIGHT_1).map(|i| (i, i));
11181118
for sacred in 0..MIN_INSERTS_HEIGHT_1 {
11191119
let mut map = BTreeMap::from_iter(pairs.clone());
1120-
map.drain_filter(|i, _| *i != sacred);
1120+
map.drain_filter(|i, _| *i != sacred).for_each(drop);
11211121
assert!(map.keys().copied().eq(sacred..=sacred));
11221122
map.check();
11231123
}
@@ -1128,7 +1128,7 @@ mod test_drain_filter {
11281128
let pairs = (0..MIN_INSERTS_HEIGHT_2).map(|i| (i, i));
11291129
for doomed in (0..MIN_INSERTS_HEIGHT_2).step_by(12) {
11301130
let mut map = BTreeMap::from_iter(pairs.clone());
1131-
map.drain_filter(|i, _| *i == doomed);
1131+
map.drain_filter(|i, _| *i == doomed).for_each(drop);
11321132
assert_eq!(map.len(), MIN_INSERTS_HEIGHT_2 - 1);
11331133
map.check();
11341134
}
@@ -1139,7 +1139,7 @@ mod test_drain_filter {
11391139
let pairs = (0..MIN_INSERTS_HEIGHT_2).map(|i| (i, i));
11401140
for sacred in (0..MIN_INSERTS_HEIGHT_2).step_by(12) {
11411141
let mut map = BTreeMap::from_iter(pairs.clone());
1142-
map.drain_filter(|i, _| *i != sacred);
1142+
map.drain_filter(|i, _| *i != sacred).for_each(drop);
11431143
assert!(map.keys().copied().eq(sacred..=sacred));
11441144
map.check();
11451145
}
@@ -1149,7 +1149,7 @@ mod test_drain_filter {
11491149
fn height_2_removing_all() {
11501150
let pairs = (0..MIN_INSERTS_HEIGHT_2).map(|i| (i, i));
11511151
let mut map = BTreeMap::from_iter(pairs);
1152-
map.drain_filter(|_, _| true);
1152+
map.drain_filter(|_, _| true).for_each(drop);
11531153
assert!(map.is_empty());
11541154
map.check();
11551155
}
@@ -1165,7 +1165,8 @@ mod test_drain_filter {
11651165
map.insert(b.spawn(Panic::InDrop), ());
11661166
map.insert(c.spawn(Panic::Never), ());
11671167

1168-
catch_unwind(move || drop(map.drain_filter(|dummy, _| dummy.query(true)))).unwrap_err();
1168+
catch_unwind(move || map.drain_filter(|dummy, _| dummy.query(true)).for_each(drop))
1169+
.unwrap_err();
11691170

11701171
assert_eq!(a.queried(), 1);
11711172
assert_eq!(b.queried(), 1);
@@ -1186,8 +1187,10 @@ mod test_drain_filter {
11861187
map.insert(b.spawn(Panic::InQuery), ());
11871188
map.insert(c.spawn(Panic::InQuery), ());
11881189

1189-
catch_unwind(AssertUnwindSafe(|| drop(map.drain_filter(|dummy, _| dummy.query(true)))))
1190-
.unwrap_err();
1190+
catch_unwind(AssertUnwindSafe(|| {
1191+
map.drain_filter(|dummy, _| dummy.query(true)).for_each(drop)
1192+
}))
1193+
.unwrap_err();
11911194

11921195
assert_eq!(a.queried(), 1);
11931196
assert_eq!(b.queried(), 1);

‎library/alloc/src/collections/btree/set.rs‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -999,7 +999,7 @@ impl<T, A: Allocator + Clone> BTreeSet<T, A> {
999999
T: Ord,
10001000
F: FnMut(&T) -> bool,
10011001
{
1002-
self.drain_filter(|v| !f(v));
1002+
self.drain_filter(|v| !f(v)).for_each(drop);
10031003
}
10041004

10051005
/// Moves all elements from `other` into `self`, leaving `other` empty.
@@ -1084,14 +1084,11 @@ impl<T, A: Allocator + Clone> BTreeSet<T, A> {
10841084
/// yielded. If the closure returns `false`, or panics, the element remains
10851085
/// in the set and will not be yielded.
10861086
///
1087-
/// If the iterator is only partially consumed or not consumed at all, each
1088-
/// of the remaining elements is still subjected to the closure and removed
1089-
/// and dropped if it returns `true`.
1090-
///
1091-
/// It is unspecified how many more elements will be subjected to the
1092-
/// closure if a panic occurs in the closure, or if a panic occurs while
1093-
/// dropping an element, or if the `DrainFilter` itself is leaked.
1087+
/// If the returned `DrainFilter` is not exhausted, e.g. because it is dropped without iterating
1088+
/// or the iteration short-circuits, then the remaining elements will be retained.
1089+
/// Use [`retain`] with a negated predicate if you do not need the returned iterator.
10941090
///
1091+
/// [`retain`]: BTreeSet::retain
10951092
/// # Examples
10961093
///
10971094
/// Splitting a set into even and odd values, reusing the original set:
@@ -1277,6 +1274,7 @@ impl<'a, T, A: Allocator + Clone> IntoIterator for &'a BTreeSet<T, A> {
12771274

12781275
/// An iterator produced by calling `drain_filter` on BTreeSet.
12791276
#[unstable(feature = "btree_drain_filter", issue = "70530")]
1277+
#[must_use = "iterators are lazy and do nothing unless consumed"]
12801278
pub struct DrainFilter<
12811279
'a,
12821280
T,
@@ -1292,16 +1290,6 @@ pub struct DrainFilter<
12921290
alloc: A,
12931291
}
12941292

1295-
#[unstable(feature = "btree_drain_filter", issue = "70530")]
1296-
impl<T, F, A: Allocator + Clone> Drop for DrainFilter<'_, T, F, A>
1297-
where
1298-
F: FnMut(&T) -> bool,
1299-
{
1300-
fn drop(&mut self) {
1301-
self.for_each(drop);
1302-
}
1303-
}
1304-
13051293
#[unstable(feature = "btree_drain_filter", issue = "70530")]
13061294
impl<T, F, A: Allocator + Clone> fmt::Debug for DrainFilter<'_, T, F, A>
13071295
where

‎library/alloc/src/collections/btree/set/tests.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,8 @@ fn test_drain_filter() {
370370
let mut x = BTreeSet::from([1]);
371371
let mut y = BTreeSet::from([1]);
372372

373-
x.drain_filter(|_| true);
374-
y.drain_filter(|_| false);
373+
x.drain_filter(|_| true).for_each(drop);
374+
y.drain_filter(|_| false).for_each(drop);
375375
assert_eq!(x.len(), 0);
376376
assert_eq!(y.len(), 1);
377377
}
@@ -387,7 +387,7 @@ fn test_drain_filter_drop_panic_leak() {
387387
set.insert(b.spawn(Panic::InDrop));
388388
set.insert(c.spawn(Panic::Never));
389389

390-
catch_unwind(move || drop(set.drain_filter(|dummy| dummy.query(true)))).ok();
390+
catch_unwind(move || set.drain_filter(|dummy| dummy.query(true)).for_each(drop)).ok();
391391

392392
assert_eq!(a.queried(), 1);
393393
assert_eq!(b.queried(), 1);
@@ -408,7 +408,10 @@ fn test_drain_filter_pred_panic_leak() {
408408
set.insert(b.spawn(Panic::InQuery));
409409
set.insert(c.spawn(Panic::InQuery));
410410

411-
catch_unwind(AssertUnwindSafe(|| drop(set.drain_filter(|dummy| dummy.query(true))))).ok();
411+
catch_unwind(AssertUnwindSafe(|| {
412+
set.drain_filter(|dummy| dummy.query(true)).for_each(drop)
413+
}))
414+
.ok();
412415

413416
assert_eq!(a.queried(), 1);
414417
assert_eq!(b.queried(), 1);

0 commit comments

Comments
 (0)