From 087be299515c2b6766c7c5dc92e853a922d172ea Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Sun, 27 Aug 2023 10:37:58 +0200 Subject: [PATCH 1/9] Replace `CompleteStateRemaining` by `Option` Known/Overflow variants have the advantage of the readability but it's less handy than options. A doc comment seems enough to me to say that None means it would overflow. --- src/permutations.rs | 44 +++++++++++--------------------------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/src/permutations.rs b/src/permutations.rs index a8885a411..51637416b 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -47,11 +47,6 @@ enum CompleteState { } } -enum CompleteStateRemaining { - Known(usize), - Overflow, -} - impl fmt::Debug for Permutations where I: Iterator + fmt::Debug, I::Item: fmt::Debug, @@ -123,12 +118,7 @@ where fn count(self) -> usize { fn from_complete(complete_state: CompleteState) -> usize { - match complete_state.remaining() { - CompleteStateRemaining::Known(count) => count, - CompleteStateRemaining::Overflow => { - panic!("Iterator count greater than usize::MAX"); - } - } + complete_state.remaining().expect("Iterator count greater than usize::MAX") } let Permutations { vals, state } = self; @@ -156,8 +146,8 @@ where PermutationState::StartUnknownLen { .. } | PermutationState::OngoingUnknownLen { .. } => (0, None), // TODO can we improve this lower bound? PermutationState::Complete(ref state) => match state.remaining() { - CompleteStateRemaining::Known(count) => (count, Some(count)), - CompleteStateRemaining::Overflow => (::std::usize::MAX, None) + Some(count) => (count, Some(count)), + None => (::std::usize::MAX, None) } PermutationState::Empty => (0, Some(0)) } @@ -238,39 +228,27 @@ impl CompleteState { } } - fn remaining(&self) -> CompleteStateRemaining { - use self::CompleteStateRemaining::{Known, Overflow}; - + /// Returns the count of remaining permutations, or None if it would overflow. + fn remaining(&self) -> Option { match *self { CompleteState::Start { n, k } => { if n < k { - return Known(0); + return Some(0); } - - let count: Option = (n - k + 1..n + 1).fold(Some(1), |acc, i| { + (n - k + 1..n + 1).fold(Some(1), |acc, i| { acc.and_then(|acc| acc.checked_mul(i)) - }); - - match count { - Some(count) => Known(count), - None => Overflow - } + }) } CompleteState::Ongoing { ref indices, ref cycles } => { let mut count: usize = 0; for (i, &c) in cycles.iter().enumerate() { let radix = indices.len() - i; - let next_count = count.checked_mul(radix) - .and_then(|count| count.checked_add(c)); - - count = match next_count { - Some(count) => count, - None => { return Overflow; } - }; + count = count.checked_mul(radix) + .and_then(|count| count.checked_add(c))?; } - Known(count) + Some(count) } } } From ff7742365f7a39be9715a4a65f2f12a82e382fc2 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Sun, 27 Aug 2023 10:44:02 +0200 Subject: [PATCH 2/9] permutations: simplify `enough_vals` Clearer. And `prefill` extending the buffer seems better performance wise than push items to the buffer one by one with `get_next`. --- src/permutations.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/permutations.rs b/src/permutations.rs index 51637416b..dbcc3e39d 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -67,14 +67,8 @@ pub fn permutations(iter: I, k: usize) -> Permutations { }; } - let mut enough_vals = true; - - while vals.len() < k { - if !vals.get_next() { - enough_vals = false; - break; - } - } + vals.prefill(k); + let enough_vals = vals.len() == k; let state = if enough_vals { PermutationState::StartUnknownLen { k } From 11d8b6bfcabdcc78b8ef5c02696e995a6910ea37 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Sun, 27 Aug 2023 11:06:39 +0200 Subject: [PATCH 3/9] size hint: `PermutationState::StartUnknownLen` We are at the very beginning. We prefilled `k` values, but did not generate any item yet. There are `n!/(n-k)!` items to come (safely done in remaining). But `n` might be unknown. --- src/permutations.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/permutations.rs b/src/permutations.rs index dbcc3e39d..d834ed8ad 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -137,7 +137,12 @@ where fn size_hint(&self) -> (usize, Option) { match self.state { - PermutationState::StartUnknownLen { .. } | + PermutationState::StartUnknownLen { k } => { + let (mut low, mut upp) = self.vals.size_hint(); + low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); + upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); + (low, upp) + } PermutationState::OngoingUnknownLen { .. } => (0, None), // TODO can we improve this lower bound? PermutationState::Complete(ref state) => match state.remaining() { Some(count) => (count, Some(count)), From 7786d2ea7ba3d036553fbd63af1d520c87312960 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Sun, 27 Aug 2023 11:15:44 +0200 Subject: [PATCH 4/9] size hint: `PermutationState::OngoingUnknownLen` We generated `prev_iteration_count` items (`n` still unknown). So the same as `PermutationState::StartUnknownLen` minus `prev_iteration_count`. --- src/permutations.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/permutations.rs b/src/permutations.rs index d834ed8ad..2c4c4fbb9 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -143,7 +143,20 @@ where upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); (low, upp) } - PermutationState::OngoingUnknownLen { .. } => (0, None), // TODO can we improve this lower bound? + PermutationState::OngoingUnknownLen { k, min_n } => { + let prev_iteration_count = min_n - k + 1; + let (mut low, mut upp) = self.vals.size_hint(); + low = CompleteState::Start { n: low, k } + .remaining() + .unwrap_or(usize::MAX) + .saturating_sub(prev_iteration_count); + upp = upp.and_then(|n| { + CompleteState::Start { n, k } + .remaining() + .map(|count| count.saturating_sub(prev_iteration_count)) + }); + (low, upp) + } PermutationState::Complete(ref state) => match state.remaining() { Some(count) => (count, Some(count)), None => (::std::usize::MAX, None) From 92d3be2c2e607b0e59a84cac708ca9e73a79e38d Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Sun, 27 Aug 2023 11:28:37 +0200 Subject: [PATCH 5/9] Test permutations' `size_hint/count` --- tests/test_std.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test_std.rs b/tests/test_std.rs index 8ea992183..3b07d1097 100644 --- a/tests/test_std.rs +++ b/tests/test_std.rs @@ -986,6 +986,32 @@ fn permutations_zero() { it::assert_equal((0..0).permutations(0), vec![vec![]]); } +#[test] +fn permutations_range_count() { + for n in 0..=7 { + for k in 0..=7 { + let len = if k <= n { + (n - k + 1..=n).product() + } else { + 0 + }; + let mut it = (0..n).permutations(k); + assert_eq!(len, it.clone().count()); + assert_eq!(len, it.size_hint().0); + assert_eq!(Some(len), it.size_hint().1); + for count in (0..len).rev() { + let elem = it.next(); + assert!(elem.is_some()); + assert_eq!(count, it.clone().count()); + assert_eq!(count, it.size_hint().0); + assert_eq!(Some(count), it.size_hint().1); + } + let should_be_none = it.next(); + assert!(should_be_none.is_none()); + } + } +} + #[test] fn combinations_with_replacement() { // Pool smaller than n From 95536d6413cb340a7c0c90ca5b3925bfa198ba24 Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Thu, 31 Aug 2023 17:58:35 +0200 Subject: [PATCH 6/9] Comment `Permutations::size_hint` --- src/permutations.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/permutations.rs b/src/permutations.rs index 2c4c4fbb9..9414a5fc8 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -138,12 +138,14 @@ where fn size_hint(&self) -> (usize, Option) { match self.state { PermutationState::StartUnknownLen { k } => { + // At the beginning, there are `n!/(n-k)!` items to come (see `remaining`) but `n` might be unknown. let (mut low, mut upp) = self.vals.size_hint(); low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); (low, upp) } PermutationState::OngoingUnknownLen { k, min_n } => { + // Same as `StartUnknownLen` minus the `prev_iteration_count` previously generated items. let prev_iteration_count = min_n - k + 1; let (mut low, mut upp) = self.vals.size_hint(); low = CompleteState::Start { n: low, k } From c12692e87b45ce1a2520d161d2a1035fa0e9adab Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Tue, 5 Sep 2023 21:46:47 +0200 Subject: [PATCH 7/9] Use `size_hint::sub_scalar` in `Permutations::size_hint` --- src/permutations.rs | 16 +++++----------- src/size_hint.rs | 1 - 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/permutations.rs b/src/permutations.rs index 9414a5fc8..ebabbd7e2 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -3,6 +3,7 @@ use std::fmt; use std::iter::once; use super::lazy_buffer::LazyBuffer; +use crate::size_hint::{self, SizeHint}; /// An iterator adaptor that iterates through all the `k`-permutations of the /// elements from an iterator. @@ -135,7 +136,7 @@ where } } - fn size_hint(&self) -> (usize, Option) { + fn size_hint(&self) -> SizeHint { match self.state { PermutationState::StartUnknownLen { k } => { // At the beginning, there are `n!/(n-k)!` items to come (see `remaining`) but `n` might be unknown. @@ -148,16 +149,9 @@ where // Same as `StartUnknownLen` minus the `prev_iteration_count` previously generated items. let prev_iteration_count = min_n - k + 1; let (mut low, mut upp) = self.vals.size_hint(); - low = CompleteState::Start { n: low, k } - .remaining() - .unwrap_or(usize::MAX) - .saturating_sub(prev_iteration_count); - upp = upp.and_then(|n| { - CompleteState::Start { n, k } - .remaining() - .map(|count| count.saturating_sub(prev_iteration_count)) - }); - (low, upp) + low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); + upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); + size_hint::sub_scalar((low, upp), prev_iteration_count) } PermutationState::Complete(ref state) => match state.remaining() { Some(count) => (count, Some(count)), diff --git a/src/size_hint.rs b/src/size_hint.rs index f7278aec9..76ccd13a2 100644 --- a/src/size_hint.rs +++ b/src/size_hint.rs @@ -30,7 +30,6 @@ pub fn add_scalar(sh: SizeHint, x: usize) -> SizeHint { /// Subtract `x` correctly from a `SizeHint`. #[inline] -#[allow(dead_code)] pub fn sub_scalar(sh: SizeHint, x: usize) -> SizeHint { let (mut low, mut hi) = sh; low = low.saturating_sub(x); From 06abc3eda3a51550a5cd9362872dbc9954cb3fec Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Tue, 5 Sep 2023 22:37:46 +0200 Subject: [PATCH 8/9] Test overflowed `Permutations::size_hint` Ideally, the lower bound of the size hint would remain `usize::MAX` but I don't see how we could make this happen. Small bugfix: `n+1` might overflow. --- src/permutations.rs | 2 +- tests/test_std.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/permutations.rs b/src/permutations.rs index ebabbd7e2..c53bf20b7 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -243,7 +243,7 @@ impl CompleteState { if n < k { return Some(0); } - (n - k + 1..n + 1).fold(Some(1), |acc, i| { + (n - k + 1..=n).fold(Some(1), |acc, i| { acc.and_then(|acc| acc.checked_mul(i)) }) } diff --git a/tests/test_std.rs b/tests/test_std.rs index 3b07d1097..77d210fb2 100644 --- a/tests/test_std.rs +++ b/tests/test_std.rs @@ -1012,6 +1012,18 @@ fn permutations_range_count() { } } +#[test] +fn permutations_overflowed_size_hints() { + let mut it = std::iter::repeat(()).permutations(2); + assert_eq!(it.size_hint().0, usize::MAX); + assert_eq!(it.size_hint().1, None); + for nb_generated in 1..=1000 { + it.next(); + assert!(it.size_hint().0 >= usize::MAX - nb_generated); + assert_eq!(it.size_hint().1, None); + } +} + #[test] fn combinations_with_replacement() { // Pool smaller than n From 65d0c60004f1a8a7f49831695f85256dc59cfbdb Mon Sep 17 00:00:00 2001 From: Philippe-Cholet Date: Tue, 5 Sep 2023 22:50:52 +0200 Subject: [PATCH 9/9] Share code in `Permutations::size_hint` --- src/permutations.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/permutations.rs b/src/permutations.rs index c53bf20b7..21a84c401 100644 --- a/src/permutations.rs +++ b/src/permutations.rs @@ -137,21 +137,18 @@ where } fn size_hint(&self) -> SizeHint { + let at_start = |k| { + // At the beginning, there are `n!/(n-k)!` items to come (see `remaining`) but `n` might be unknown. + let (mut low, mut upp) = self.vals.size_hint(); + low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); + upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); + (low, upp) + }; match self.state { - PermutationState::StartUnknownLen { k } => { - // At the beginning, there are `n!/(n-k)!` items to come (see `remaining`) but `n` might be unknown. - let (mut low, mut upp) = self.vals.size_hint(); - low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); - upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); - (low, upp) - } + PermutationState::StartUnknownLen { k } => at_start(k), PermutationState::OngoingUnknownLen { k, min_n } => { - // Same as `StartUnknownLen` minus the `prev_iteration_count` previously generated items. - let prev_iteration_count = min_n - k + 1; - let (mut low, mut upp) = self.vals.size_hint(); - low = CompleteState::Start { n: low, k }.remaining().unwrap_or(usize::MAX); - upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); - size_hint::sub_scalar((low, upp), prev_iteration_count) + // Same as `StartUnknownLen` minus the previously generated items. + size_hint::sub_scalar(at_start(k), min_n - k + 1) } PermutationState::Complete(ref state) => match state.remaining() { Some(count) => (count, Some(count)),