-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge Merge/MergeBy/MergeJoinBy
implementations (v2)
#736
Merge Merge/MergeBy/MergeJoinBy
implementations (v2)
#736
Conversation
Merge/MergeBy/MergeJoinBy
implementationsMerge/MergeBy/MergeJoinBy
implementations (v2)
@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`.
6061c96
to
2ce8986
Compare
Rebased after recent #751. |
Apparently not needed anymore.
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. |
Cleaner alternative to the experimental/messy #711. Closes #701.
@phimuemue I would additionally rename
InternalMergeJoinBy
toMergeBy
, makingMergeJoinBy
an alias ofMergeBy
instead. 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 withMergeFuncT
anymore.