From ecebdf396bd12ad4ab51a251f275cda8bfb97b3b Mon Sep 17 00:00:00 2001 From: Clint White <104277906+clint-white@users.noreply.github.com> Date: Sun, 1 May 2022 11:51:12 -0400 Subject: [PATCH 1/5] Relax some arithmetic trait bounds on type parameter `N` The first `impl` block contained several methods which really have different needs as far as trait bounds on `N` are concerned, forcing the block to have the union of all of those requirements. By breaking this `impl` block up we can better limit the bounds for each method to what is strictly necessary. This has downstream benefits in relaxing the bounds on other trait implementations further down in the crate. For example, `Counter::update()` does not require `N: PartialOrd` or `N: SubAssign`, `Counter::subtract()` does not require `N: AddAssign`, and `Counter::new()` requires neither the `PartialOrd` nor `AddAssign` nor `SubAssign` nor `One` bounds on `N`. This also eliminates the counter-intuitive bound `N: SubAssign` on the implementation of `AddAssign` for `Counter`, as well as the bound `N: AddAssign` on the implementation of `SubAssign` for `Counter`, etc. --- src/lib.rs | 98 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 55 insertions(+), 43 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b4e3c7d..e98d000 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -175,7 +175,17 @@ pub struct Counter { impl Counter where T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, +{ + /// Consumes this counter and returns a HashMap mapping the items to the counts. + pub fn into_map(self) -> HashMap { + self.map + } +} + +impl Counter +where + T: Hash + Eq, + N: Zero, { /// Create a new, empty `Counter` pub fn new() -> Counter { @@ -185,6 +195,34 @@ where } } + /// Returns the sum of the counts. + /// + /// Use [`len`] to get the number of elements in the counter and use `total` to get the sum of + /// their counts. + /// + /// [`len`]: struct.Counter.html#method.len + /// + /// # Examples + /// + /// ``` + /// # use counter::Counter; + /// let counter = Counter::init("abracadabra".chars()); + /// assert_eq!(counter.total::(), 11); + /// assert_eq!(counter.len(), 5); + /// ``` + pub fn total<'a, S>(&'a self) -> S + where + S: iter::Sum<&'a N>, + { + self.map.values().sum() + } +} + +impl Counter +where + T: Hash + Eq, + N: AddAssign + Zero + One, +{ /// Create a new `Counter` initialized with the given iterable pub fn init(iterable: I) -> Counter where @@ -205,12 +243,13 @@ where *entry += N::one(); } } +} - /// Consumes this counter and returns a HashMap mapping the items to the counts. - pub fn into_map(self) -> HashMap { - self.map - } - +impl Counter +where + T: Hash + Eq, + N: PartialOrd + SubAssign + Zero + One, +{ /// Remove the counts of the elements from the given iterable to this counter /// /// Non-positive counts are automatically removed @@ -318,33 +357,6 @@ where } } -impl Counter -where - T: Hash + Eq, -{ - /// Returns the sum of the counts. - /// - /// Use [`len`] to get the number of elements in the counter and use `total` to get the sum of - /// their counts. - /// - /// [`len`]: struct.Counter.html#method.len - /// - /// # Examples - /// - /// ``` - /// # use counter::Counter; - /// let counter = Counter::init("abracadabra".chars()); - /// assert_eq!(counter.total::(), 11); - /// assert_eq!(counter.len(), 5); - /// ``` - pub fn total<'a, S>(&'a self) -> S - where - S: iter::Sum<&'a N>, - { - self.map.values().sum() - } -} - impl Default for Counter where T: Hash + Eq, @@ -487,7 +499,7 @@ where impl BitAnd for Counter where T: Clone + Hash + Eq, - N: Clone + Ord + AddAssign + SubAssign + Zero + One, + N: Clone + Ord + Zero, { type Output = Counter; @@ -725,7 +737,7 @@ impl AddAssign for Counter where I: IntoIterator, T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero + One, { /// Directly add the counts of the elements of `I` to `self` /// @@ -748,7 +760,7 @@ impl Add for Counter where I: IntoIterator, T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero + One, { type Output = Self; /// Consume self producing a Counter like self updated with the counts of @@ -774,7 +786,7 @@ impl SubAssign for Counter where I: IntoIterator, T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: PartialOrd + SubAssign + Zero + One, { /// Directly subtract the counts of the elements of `I` from `self`, /// keeping only items with a value greater than N::zero(). @@ -797,7 +809,7 @@ impl Sub for Counter where I: IntoIterator, T: Clone + Hash + Eq, - N: Clone + PartialOrd + AddAssign + SubAssign + Zero + One, + N: Clone + PartialOrd + SubAssign + Zero + One, { type Output = Self; /// Consume self producing a Counter like self with the counts of the @@ -821,7 +833,7 @@ where impl iter::FromIterator for Counter where T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero + One, { /// Produce a Counter from an iterator of items. This is called automatically /// by `iter.collect()`. @@ -842,7 +854,7 @@ where impl iter::FromIterator<(T, N)> for Counter where T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero, { /// `from_iter` creates a counter from `(item, count)` tuples. /// @@ -869,7 +881,7 @@ where impl Extend for Counter where T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero + One, { /// Extend a Counter with an iterator of items. /// @@ -889,7 +901,7 @@ where impl Extend<(T, N)> for Counter where T: Hash + Eq, - N: PartialOrd + AddAssign + SubAssign + Zero + One, + N: AddAssign + Zero, { /// Extend a counter with `(item, count)` tuples. /// @@ -914,7 +926,7 @@ where impl<'a, T: 'a, N: 'a> Extend<(&'a T, &'a N)> for Counter where T: Hash + Eq + Copy, - N: PartialOrd + AddAssign + SubAssign + Zero + One + Copy, + N: AddAssign + Zero + Copy, { /// Extend a counter with `(item, count)` tuples. /// From 030266656cd18a960b3211210f457bf1d3596c18 Mon Sep 17 00:00:00 2001 From: Clint White <104277906+clint-white@users.noreply.github.com> Date: Sun, 1 May 2022 12:12:06 -0400 Subject: [PATCH 2/5] Relax some bounds unnecessarily requiring `Clone` The implementations of `Deref` and `DerefMut` do not clone `N` and hence do not need the bound `N: Clone`. It looks like the bound was added in commit cb24e7f4 because it was required by the bound `N: Clone` on the struct itself. The bound on the struct was later removed in 8180199a, but it looks like the bound on these `impl`s remained. The current implementation of `Sub>` does not clone either `T` or `N` and hence does not need either bound `T: Clone` or `N: Clone`. It looks like the original implementation did clone `self`, but that was changed in cdaad73fs at which point the bound `T: Clone` and `N: Clone` could have been removed. --- src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e98d000..d9e1ce3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -574,7 +574,6 @@ where impl Deref for Counter where T: Hash + Eq, - N: Clone, { type Target = CounterMap; fn deref(&self) -> &CounterMap { @@ -585,7 +584,6 @@ where impl DerefMut for Counter where T: Hash + Eq, - N: Clone, { fn deref_mut(&mut self) -> &mut CounterMap { &mut self.map @@ -808,8 +806,8 @@ where impl Sub for Counter where I: IntoIterator, - T: Clone + Hash + Eq, - N: Clone + PartialOrd + SubAssign + Zero + One, + T: Hash + Eq, + N: PartialOrd + SubAssign + Zero + One, { type Output = Self; /// Consume self producing a Counter like self with the counts of the From 5b4309ee7720b69578df97e6fd30ee1969d435a4 Mon Sep 17 00:00:00 2001 From: Clint White <104277906+clint-white@users.noreply.github.com> Date: Sun, 1 May 2022 13:42:55 -0400 Subject: [PATCH 3/5] Avoid unnecessary use of `Clone` This builds on commit c2d71986, which refactored the iteration to take ownership of the keys and values of `rhs.map` instead of references. Not only is `value` owned and hence does not need to be cloned, but also `key` is owned and thus there is no need to clone it to look up the entry. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d9e1ce3..91dfb9f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -372,7 +372,7 @@ where impl AddAssign for Counter where - T: Clone + Hash + Eq, + T: Hash + Eq, N: Zero + AddAssign, { /// Add another counter to this counter @@ -392,7 +392,7 @@ where /// ``` fn add_assign(&mut self, rhs: Self) { for (key, value) in rhs.map.into_iter() { - let entry = self.map.entry(key.clone()).or_insert_with(N::zero); + let entry = self.map.entry(key).or_insert_with(N::zero); *entry += value; } } From bedc65aa4f3b47120eb7c996fb3547aaf5092723 Mon Sep 17 00:00:00 2001 From: Clint White <104277906+clint-white@users.noreply.github.com> Date: Sun, 1 May 2022 14:21:18 -0400 Subject: [PATCH 4/5] Refactor impls of `BitAnd`, `BitOr` to avoid cloning Since we are taking ownership of both the `self` and `rhs` Counters, we really do not have to clone either keys or values to produce new Counters with the correct semantics for these traits. This commit refactors these implementations to void cloning and removes the bounds `T: Clone` and `N: Clone`. --- src/lib.rs | 52 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 91dfb9f..4488249 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -498,8 +498,8 @@ where impl BitAnd for Counter where - T: Clone + Hash + Eq, - N: Clone + Ord + Zero, + T: Hash + Eq, + N: Ord + Zero, { type Output = Counter; @@ -518,30 +518,24 @@ where /// let expect = [('a', 1), ('b', 1)].iter().cloned().collect::>(); /// assert_eq!(e.into_map(), expect); /// ``` - fn bitand(self, rhs: Counter) -> Self::Output { + fn bitand(self, mut rhs: Counter) -> Self::Output { use std::cmp::min; - use std::collections::HashSet; - - let self_keys = self.map.keys().collect::>(); - let other_keys = rhs.map.keys().collect::>(); - let both_keys = self_keys.intersection(&other_keys); let mut counter = Counter::new(); - for key in both_keys { - counter.map.insert( - (*key).clone(), - min(self.map.get(*key).unwrap(), rhs.map.get(*key).unwrap()).clone(), - ); + for (key, lhs_count) in self.map { + if let Some(rhs_count) = rhs.remove(&key) { + let count = min(lhs_count, rhs_count); + counter.map.insert(key, count); + } } - counter } } impl BitOr for Counter where - T: Clone + Hash + Eq, - N: Clone + Ord + Zero, + T: Hash + Eq, + N: Ord + Zero, { type Output = Counter; @@ -561,11 +555,27 @@ where /// assert_eq!(e.into_map(), expect); /// ``` fn bitor(mut self, rhs: Counter) -> Self::Output { - use std::cmp::max; - - for (key, value) in rhs.map.iter() { - let entry = self.map.entry(key.clone()).or_insert_with(N::zero); - *entry = max(&*entry, value).clone(); + for (key, rhs_value) in rhs.map { + let entry = self.map.entry(key).or_insert_with(N::zero); + // We want to update the value of the now occupied entry in `self` with the maximum of + // its current value and `rhs_value`. If that max is `rhs_value`, we can just update + // the value of the entry. If the max is the current value, we do nothing. Note that + // `Ord::max()` returns the second argument (here `rhs_value`) if its two arguments are + // equal, justifying the use of the weak inequality below instead of a strict + // inequality. + // + // Doing it this way with an inequality instead of actually using `std::cmp::max()` + // lets us avoid trying (and failing) to move the non-copy value out of the entry in + // order to pass it as an argument to `std::cmp::max()`, while still holding a mutable + // reference to the value slot in the entry. + // + // And while using the inequality seemingly only requires the bound `N: PartialOrd`, we + // nevertheless prefer to require `Ord` as though we were using `std::cmp::max()` + // because the semantics of `BitOr` for `Counter` really do not make sense if there are + // possibly non-comparable values of type `N`. + if rhs_value >= *entry { + *entry = rhs_value; + } } self } From 1d49f69b4a9d73167627203f3f172900f41f2da4 Mon Sep 17 00:00:00 2001 From: Clint White <104277906+clint-white@users.noreply.github.com> Date: Sun, 1 May 2022 14:46:41 -0400 Subject: [PATCH 5/5] Change `Copy` bound to `Clone` Elsewhere in the crate when we have a borrow but need ownership, we clone instead of copy. It seems it would be more consistent to do the same here. --- src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 4488249..66f26fe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -933,8 +933,8 @@ where impl<'a, T: 'a, N: 'a> Extend<(&'a T, &'a N)> for Counter where - T: Hash + Eq + Copy, - N: AddAssign + Zero + Copy, + T: Hash + Eq + Clone, + N: AddAssign + Zero + Clone, { /// Extend a counter with `(item, count)` tuples. /// @@ -951,8 +951,8 @@ where /// ``` fn extend>(&mut self, iter: I) { for (item, item_count) in iter.into_iter() { - let entry = self.map.entry(*item).or_insert_with(N::zero); - *entry += *item_count; + let entry = self.map.entry(item.clone()).or_insert_with(N::zero); + *entry += item_count.clone(); } } }