search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

Merge `Merge/MergeBy/MergeJoinBy` implementations #711

Closed Philippe-Cholet closed 1 year ago

Philippe-Cholet commented 1 year ago

Merge and MergeBy are moved from "adaptors" to "merge_join" modules. Then Merge, MergeBy and MergeJoinBy are now instances of a new (common and private) struct: InternalMergeJoinBy. Internally, instead of OrderingOrBool and FnMut(&L, &R) -> Out, we use MergePredicate<L, R, Out=...> (Out being Ordering or bool). And with MergeFuncT and MergeFuncLR also kept private, the functions return those types:

merge                      -> InternalMergeJoinBy<I, J, MergeFuncT<MergeLte>>
merge_by                   -> InternalMergeJoinBy<I, J, MergeFuncT<F>>
merge_join_by(...bool)     -> InternalMergeJoinBy<I, J, MergeFuncLR<F, bool>>
merge_join_by(...Ordering) -> InternalMergeJoinBy<I, J, MergeFuncLR<F, Ordering>>

The only changes for an external user:

Closes #701

PS: In fn merge_join_by, the constraint T: OrderingOrBool<I::Item, J::Item>, has been removed. Even if the documentation is clear, we could add it back in some way if you want.

phimuemue commented 1 year ago

Hi @Philippe-Cholet, thanks for this. I try to pick this up, but I am very unsure that MergeJoinBy really needs a fourth type parameter, so I for now only cherry-picked the first commit here.

Philippe-Cholet commented 1 year ago

@phimuemue A bit disappointed, I started again and to my own surprise I quickly found a way (fresh eyes I guess). So https://github.com/rust-itertools/itertools/pull/711/commits/0a3f122a58cb57c478b4813887e1cfc5a08c0e25 It results in a big type definition but it works.

Philippe-Cholet commented 1 year ago

I'm closing this. This is the result of some useful experiments but became too messy to be properly reviewed and it deserves a cleaner PR I'm currently working on. At least, the first commit has been cherry-picked.