From 9b981de40812dd3ea811111ccd0cc782e6a596f6 Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Sat, 18 Jul 2026 19:20:20 +1000 Subject: [PATCH 1/6] fix: PartialEq and PartialOrd traits use correct multiset partial ordering semantics - PartialEq now compares keys using the implici 'zero' element, rather than using equality on the underlying map. - PartialOrd is now implemented to allow for the <, >, <=, >= operator overloads. Migrated is_subset and is_superset to wrap around the ParitialOrd::partial_cmp method. --- src/lib.rs | 51 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 17f31b6..7917e3d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -279,6 +279,7 @@ mod impls; use num_traits::{One, Zero}; +use std::cmp::Ordering; use std::collections::{BinaryHeap, HashMap}; use std::hash::{BuildHasher, Hash, RandomState}; use std::iter; @@ -296,19 +297,37 @@ pub struct Counter { impl PartialEq for Counter where T: Eq + Hash, - N: PartialEq, + N: PartialEq + Zero, S: BuildHasher, { fn eq(&self, other: &Self) -> bool { - // ignore the zero - self.map == other.map + self.keys().chain(other.keys()).all(|k| self[k] == other[k]) + } +} + +impl PartialOrd for Counter +where + T: Eq + Hash, + N: PartialOrd + Zero, + S: BuildHasher, +{ + fn partial_cmp(&self, other: &Self) -> Option { + let keys = self.keys().chain(other.keys()); + let le = keys.clone().all(|k| self[k] <= other[k]); + let ge = keys.clone().all(|k| self[k] >= other[k]); + match (le, ge) { + (true, true) => Some(Ordering::Equal), + (true, false) => Some(Ordering::Less), + (false, true) => Some(Ordering::Greater), + (false, false) => None, + } } } impl Eq for Counter where T: Eq + Hash, - N: Eq, + N: Eq + Zero, S: BuildHasher, { } @@ -570,8 +589,6 @@ where /// This is true if for all elements in this counter and the other, /// the count in this counter is greater than or equal to the count in the other. /// - /// `c.is_superset(&d);` -> `c.iter().all(|(x, n)| n >= d[x]) && d.iter().all(|(x, n)| c[x] >= n)` - /// /// ```rust /// # use counter::Counter; /// # use std::collections::HashMap; @@ -583,20 +600,16 @@ where /// assert!(!c.is_superset(&d)); /// ``` pub fn is_superset(&self, other: &Self) -> bool { - // need to test keys from both counters, because if N is signed, counts in `self` - // could be < 0 for elements missing in `other`. For the unsigned case, only elements - // from `other` would need to be tested. - self.keys() - .chain(other.keys()) - .all(|key| self[key] >= other[key]) + match self.partial_cmp(other) { + Some(Ordering::Greater | Ordering::Equal) => true, + _ => false, + } } /// Test whether this counter is a subset of another counter. /// This is true if for all elements in this counter and the other, /// the count in this counter is less than or equal to the count in the other. /// - /// `c.is_subset(&d);` -> `c.iter().all(|(x, n)| n <= d[x]) && d.iter().all(|(x, n)| c[x] <= n)` - /// /// ```rust /// # use counter::Counter; /// # use std::collections::HashMap; @@ -608,11 +621,9 @@ where /// assert!(!c.is_subset(&d)); /// ``` pub fn is_subset(&self, other: &Self) -> bool { - // need to test keys from both counters, because if N is signed, counts in `other` - // could be < 0 for elements missing in `self`. For the unsigned case, only elements - // from `self` would need to be tested. - self.keys() - .chain(other.keys()) - .all(|key| self[key] <= other[key]) + match self.partial_cmp(other) { + Some(Ordering::Less | Ordering::Equal) => true, + _ => false, + } } } From e31c8ba4359ea2698b56a93aa3782e1afb7e4c8f Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Sat, 18 Jul 2026 19:20:47 +1000 Subject: [PATCH 2/6] test: add a small unit test for PartialOrd semantics --- src/unit_tests.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/unit_tests.rs b/src/unit_tests.rs index b9d61ff..8c40771 100644 --- a/src/unit_tests.rs +++ b/src/unit_tests.rs @@ -239,3 +239,12 @@ fn test_non_usize_count() { }; assert!(counter.map == expected); } + +#[test] +fn test_superset() { + let c1: Counter<_> = "ab".chars().collect(); + let c2: Counter<_> = "a".chars().collect(); + assert_ne!(c1, c2); + assert!(c1 >= c2); + assert!(c1 > c2); +} From 450bbbe7aa2cc64f21937a30ca90e04074cee00e Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Tue, 28 Jul 2026 17:11:19 +1000 Subject: [PATCH 3/6] refactor: re-use PartialOrd impl for is_superset and is_subset methods --- src/lib.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 7917e3d..fa8c4ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -600,10 +600,7 @@ where /// assert!(!c.is_superset(&d)); /// ``` pub fn is_superset(&self, other: &Self) -> bool { - match self.partial_cmp(other) { - Some(Ordering::Greater | Ordering::Equal) => true, - _ => false, - } + self >= other } /// Test whether this counter is a subset of another counter. @@ -621,9 +618,6 @@ where /// assert!(!c.is_subset(&d)); /// ``` pub fn is_subset(&self, other: &Self) -> bool { - match self.partial_cmp(other) { - Some(Ordering::Less | Ordering::Equal) => true, - _ => false, - } + self <= other } } From 5595a76629ab4098a884b19ab3d4c294b94ce5c6 Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Tue, 28 Jul 2026 17:14:51 +1000 Subject: [PATCH 4/6] fix: restore doc comments for is_subset and is_superset methods --- src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index fa8c4ec..e8e36dc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -589,6 +589,8 @@ where /// This is true if for all elements in this counter and the other, /// the count in this counter is greater than or equal to the count in the other. /// + /// `c.is_superset(&d);` -> `c.iter().all(|(x, n)| n >= d[x]) && d.iter().all(|(x, n)| c[x] >= n)` + /// /// ```rust /// # use counter::Counter; /// # use std::collections::HashMap; @@ -607,6 +609,8 @@ where /// This is true if for all elements in this counter and the other, /// the count in this counter is less than or equal to the count in the other. /// + /// `c.is_subset(&d);` -> `c.iter().all(|(x, n)| n <= d[x]) && d.iter().all(|(x, n)| c[x] <= n)` + /// /// ```rust /// # use counter::Counter; /// # use std::collections::HashMap; From 46386c1f5f95e23dabc9d874c8fffe57a7527655 Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Tue, 28 Jul 2026 17:24:39 +1000 Subject: [PATCH 5/6] refactor: replace double pass over keys with single pass --- src/lib.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e8e36dc..01cbbee 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -312,9 +312,13 @@ where S: BuildHasher, { fn partial_cmp(&self, other: &Self) -> Option { - let keys = self.keys().chain(other.keys()); - let le = keys.clone().all(|k| self[k] <= other[k]); - let ge = keys.clone().all(|k| self[k] >= other[k]); + let (mut le, mut ge) = (true, true); + + for key in self.keys().chain(other.keys()) { + le &= self[key] <= other[key]; + ge &= self[key] >= other[key]; + } + match (le, ge) { (true, true) => Some(Ordering::Equal), (true, false) => Some(Ordering::Less), From 5cb7c391ea05d269e3989a2eee9d33ef0b2c29da Mon Sep 17 00:00:00 2001 From: Yossi Frenkel Date: Tue, 28 Jul 2026 17:27:40 +1000 Subject: [PATCH 6/6] nit: cargo fmt --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 01cbbee..6204ec4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -315,10 +315,10 @@ where let (mut le, mut ge) = (true, true); for key in self.keys().chain(other.keys()) { - le &= self[key] <= other[key]; + le &= self[key] <= other[key]; ge &= self[key] >= other[key]; } - + match (le, ge) { (true, true) => Some(Ordering::Equal), (true, false) => Some(Ordering::Less),