Relax trait bounds #21
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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<T, N>`, as well as the bound `N: AddAssign` on the implementation of `SubAssign` for `Counter<T, N>`, etc.
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 cb24e7f because it was required by the bound `N: Clone` on the struct itself. The bound on the struct was later removed in 8180199, but it looks like the bound on these `impl`s remained. The current implementation of `Sub<I: IntoIterator<Item = T>>` 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.
This builds on commit c2d7198, 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.
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`.
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.
|
At first glance this looks plausible, but I'm busy at the moment and don't have time to give this a proper review. I intend to come back to this within a week. If next Sunday I haven't actually properly reviewed this, please ping me. |
coriolinus
approved these changes
May 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are some proposed changes that collectively relax some of the trait bounds in the crate. They are split into several commits so that each one can be coherent and focussed on one kind of change and in that way, hopefully, easier to review. In some cases existing bounds can be relaxed without modifying code in the implementations; in others a minor amount of refactoring allows us to relax the bounds.
This builds on similar work in prior PRs such as #7, #17, #19.