Merge Merge/MergeBy/MergeJoinBy implementations (v2)
#736
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
Philippe-Cholet
changed the title
Merge/MergeBy/MergeJoinBy implementationsMerge/MergeBy/MergeJoinBy implementations (v2)
jswrenn
requested changes
Aug 31, 2023
|
@phimuemue What do you think of this new version? |
And the trait `OrderingOrBool<L, R>` is now for wrapped functions rather than for `Ordering` and `bool`. NOTES: - The new struct `MergeFuncLR<F, T>` is useful to avoid conflicting implementations of `OrderingOrBool<L, R> for F` because the compiler thinks that `F` could implement both `FnMut(&L, &R) -> Ordering` and `FnMut(&L, &R) -> bool`. - The new trait `FuncLR<L, R>` is useful to be able to infer the Output type `T` without having to add a fourth parameter to `MergeJoinBy`. From what I understand, this is needed because the `FnMut` trait is not fully stabilized yet. - `OrderingOrBool<L, R>` has a new associated type `Out`, which is useful in `impl Iterator` because otherwise the compiler would complain about `T` be unconstrained. This successfully pass the tests.
Being now an alias, we can remove various implementations. `FusedIterator` will be inserted back soon. We don't need `MergePredicate<T>` anymore because we use two new implementations of `OrderingOrBool<T, T>`.
`I` and `J` are fused by `InternalMergeJoinBy` so we don't need them to implement `FusedIterator`.
Philippe-Cholet
force-pushed
the
issue-701-part2-v2
branch
from
6061c96
to
2ce8986
Compare
|
Rebased after recent #751. |
jswrenn
requested changes
Sep 20, 2023
Apparently not needed anymore.
jswrenn
approved these changes
Sep 20, 2023
|
That feature was quite a ride for me. 🎉 @jswrenn Note that you are mixing different PRs with the "next" milestone. |
|
What do you mean by "mixing"? We use the |
|
Just that old PRs of 0.11 are with the new ones, which I noticed and found weird but if it's not an issue then case closed. |
Merged
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.
Cleaner alternative to the experimental/messy #711. Closes #701.
@phimuemue I would additionally rename
InternalMergeJoinBytoMergeBy, makingMergeJoinByan alias ofMergeByinstead. But I would like your opinion on this first.I recently found a way to not add a fourth type to
MergeJoinBy(added to the old PR) but another difference is that I do not wrap withMergeFuncTanymore.