Update Merge and MergeBy: avoid function pointers

Refactor common parts of merge into MergeCore

For issue #49

Author: bluss

benches/bench1.rs

@@ -14,7 +14,6 @@ use itertools::ZipSlices;
 
 use std::iter::repeat;
 use std::cmp;
-use std::cmp::Ordering;
 
 #[bench]
 fn slice_iter(b: &mut test::Bencher)
@@ -609,7 +608,7 @@ fn merge_by_cmp(b: &mut test::Bencher) {
     let data1 = test::black_box(data1);
     let data2 = test::black_box(data2);
     b.iter(|| {
-        data1.iter().merge_by(&data2, Ord::cmp).count()
+        data1.iter().merge_by(&data2, PartialOrd::le).count()
     })
 }
 
@@ -635,6 +634,6 @@ fn merge_by_lt(b: &mut test::Bencher) {
     let data1 = test::black_box(data1);
     let data2 = test::black_box(data2);
     b.iter(|| {
-        data1.iter().merge_by(&data2, |a, b| if a <= b { Ordering::Less } else { Ordering::Greater }).count()
+        data1.iter().merge_by(&data2, |a, b| a <= b).count()
     })
 }

src/adaptors.rs

@@ -10,7 +10,6 @@ use std::mem;
 use std::num::One;
 #[cfg(feature = "unstable")]
 use std::ops::Add;
-use std::cmp::Ordering;
 use std::iter::{Fuse, Peekable};
 use std::collections::HashSet;
 use std::hash::Hash;
@@ -504,80 +503,39 @@ impl<I> ExactSizeIterator for Step<I> where
     I: ExactSizeIterator,
 { }
 
-/// An iterator adaptor that merges the two base iterators in ascending order.
-/// If both base iterators are sorted (ascending), the result is sorted.
-///
-/// Iterator element type is `I::Item`.
-///
-/// See [*.merge_by()*](trait.Itertools.html#method.merge_by) for more information.
-pub struct Merge<I, J, F> where
-    I: Iterator,
-    J: Iterator<Item=I::Item>,
+
+struct MergeCore<I, J>
+    where I: Iterator,
+          J: Iterator<Item=I::Item>,
 {
     a: Peekable<I>,
     b: Peekable<J>,
-    cmp: F,
     fused: Option<bool>,
 }
 
-// default ordering function for .merge()
-// Note: To be replaced by unit struct
-#[inline]
-pub fn merge_default_ordering<A: PartialOrd>(a: &A, b: &A) -> Ordering {
-    if a > b {
-        Ordering::Greater
-    } else {
-        Ordering::Less
-    }
-}
-
-impl<I, J, F> Merge<I, J, F> where
-    I: Iterator,
-    J: Iterator<Item=I::Item>,
-    F: FnMut(&I::Item, &I::Item) -> Ordering
-{
-    /// Create a `Merge` iterator.
-    pub fn new(a: I, b: J, cmp: F) -> Self
-    {
-        Merge {
-            a: a.peekable(),
-            b: b.peekable(),
-            cmp: cmp,
-            fused: None,
-        }
-    }
-}
 
-impl<I, J, F> Clone for Merge<I, J, F> where
+impl<I, J> Clone for MergeCore<I, J> where
     I: Iterator,
     J: Iterator<Item=I::Item>,
     Peekable<I>: Clone,
     Peekable<J>: Clone,
-    F: Clone,
 {
     fn clone(&self) -> Self {
-        clone_fields!(Merge, self, a, b, cmp, fused)
+        clone_fields!(MergeCore, self, a, b, fused)
     }
 }
 
-impl<I, J, F> Iterator for Merge<I, J, F> where
-    I: Iterator,
-    J: Iterator<Item=I::Item>,
-    F: FnMut(&I::Item, &I::Item) -> Ordering
+impl<I, J> MergeCore<I, J>
+    where I: Iterator,
+          J: Iterator<Item=I::Item>,
 {
-    type Item = I::Item;
-
-    fn next(&mut self) -> Option<I::Item> {
+    fn next_with<F>(&mut self, mut less_than: F) -> Option<I::Item>
+        where F: FnMut(&I::Item, &I::Item) -> bool
+    {
         let less_than = match self.fused {
             Some(lt) => lt,
             None => match (self.a.peek(), self.b.peek()) {
-                (Some(a), Some(b)) => {
-                    match (self.cmp)(a, b) {
-                        Ordering::Less => true,
-                        Ordering::Equal => true,
-                        Ordering::Greater => false,
-                    }
-                }
+                (Some(a), Some(b)) => less_than(a, b),
                 (Some(_), None) => {
                     self.fused = Some(true);
                     true
@@ -603,6 +561,117 @@ impl<I, J, F> Iterator for Merge<I, J, F> where
     }
 }
 
+/// An iterator adaptor that merges the two base iterators in ascending order.
+/// If both base iterators are sorted (ascending), the result is sorted.
+///
+/// Iterator element type is `I::Item`.
+///
+/// See [*.merge()*](trait.Itertools.html#method.merge_by) for more information.
+pub struct Merge<I, J> where
+    I: Iterator,
+    J: Iterator<Item=I::Item>,
+{
+    merge: MergeCore<I, J>,
+}
+
+impl<I, J> Clone for Merge<I, J> where
+    I: Iterator,
+    J: Iterator<Item=I::Item>,
+    Peekable<I>: Clone,
+    Peekable<J>: Clone,
+{
+    fn clone(&self) -> Self {
+        clone_fields!(Merge, self, merge)
+    }
+}
+
+/// Create a `Merge` iterator.
+pub fn merge_new<I, J>(a: I, b: J) -> Merge<I, J>
+    where I: Iterator,
+          J: Iterator<Item=I::Item>,
+{
+    Merge {
+        merge: MergeCore {
+            a: a.peekable(),
+            b: b.peekable(),
+            fused: None,
+        }
+    }
+}
+
+impl<I, J> Iterator for Merge<I, J>
+    where I: Iterator,
+          J: Iterator<Item=I::Item>,
+          I::Item: PartialOrd,
+{
+    type Item = I::Item;
+
+    fn next(&mut self) -> Option<I::Item> {
+        self.merge.next_with(|a, b| a <= b)
+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        self.merge.size_hint()
+    }
+}
+
+/// An iterator adaptor that merges the two base iterators in ascending order.
+/// If both base iterators are sorted (ascending), the result is sorted.
+///
+/// Iterator element type is `I::Item`.
+///
+/// See [*.merge_by()*](trait.Itertools.html#method.merge_by) for more information.
+pub struct MergeBy<I, J, F> where
+    I: Iterator,
+    J: Iterator<Item=I::Item>,
+{
+    merge: MergeCore<I, J>,
+    cmp: F,
+}
+
+/// Create a `MergeBy` iterator.
+pub fn merge_by_new<I, J, F>(a: I, b: J, cmp: F) -> MergeBy<I, J, F>
+    where I: Iterator,
+          J: Iterator<Item=I::Item>,
+{
+    MergeBy {
+        merge: MergeCore {
+            a: a.peekable(),
+            b: b.peekable(),
+            fused: None,
+        },
+        cmp: cmp,
+    }
+}
+
+impl<I, J, F> Clone for MergeBy<I, J, F> where
+    I: Iterator,
+    J: Iterator<Item=I::Item>,
+    Peekable<I>: Clone,
+    Peekable<J>: Clone,
+    F: Clone,
+{
+    fn clone(&self) -> Self {
+        clone_fields!(MergeBy, self, merge, cmp)
+    }
+}
+
+impl<I, J, F> Iterator for MergeBy<I, J, F> where
+    I: Iterator,
+    J: Iterator<Item=I::Item>,
+    F: FnMut(&I::Item, &I::Item) -> bool
+{
+    type Item = I::Item;
+
+    fn next(&mut self) -> Option<I::Item> {
+        self.merge.next_with(&mut self.cmp)
+    }
+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        self.merge.size_hint()
+    }
+}
+
 #[cfg(feature = "unstable")]
 /// An iterator adaptor that enumerates the iterator elements,
 /// with a custom starting value and integer type.
@@ -787,8 +856,6 @@ impl<I, F> Iterator for Coalesce<I, F> where
     }
 }
 
-
-
 /// An iterator adaptor that borrows from a `Clone`-able iterator
 /// to only pick off elements while the predicate returns `true`.
 ///

src/lib.rs

@@ -51,6 +51,7 @@ pub use adaptors::{
     GroupBy,
     Step,
     Merge,
+    MergeBy,
     MultiPeek,
     TakeWhileRef,
     WhileSome,
@@ -105,9 +106,6 @@ mod zipslices;
 /// The function pointer map iterator created with `.map_fn()`.
 pub type MapFn<I, B> where I: Iterator = iter::Map<I, fn(I::Item) -> B>;
 
-/// An ascending order merge iterator created with `.merge()`.
-pub type MergeAscend<I, J> where I: Iterator = Merge<I, J, fn(&I::Item, &I::Item) -> Ordering>;
-
 #[macro_export]
 /// Create an iterator over the “cartesian product” of iterators.
 ///
@@ -521,12 +519,12 @@ pub trait Itertools : Iterator {
     /// let it = a.merge(b);
     /// itertools::assert_equal(it, vec![0, 0, 3, 5, 6, 9, 10]);
     /// ```
-    fn merge<J>(self, other: J) -> MergeAscend<Self, J::IntoIter> where
+    fn merge<J>(self, other: J) -> Merge<Self, J::IntoIter> where
         Self: Sized,
         Self::Item: PartialOrd,
         J: IntoIterator<Item=Self::Item>,
     {
-        self.merge_by(other, adaptors::merge_default_ordering)
+        adaptors::merge_new(self, other.into_iter())
     }
 
     /// Return an iterator adaptor that merges the two base iterators in order.
@@ -541,16 +539,16 @@ pub trait Itertools : Iterator {
     ///
     /// let a = (0..).zip("bc".chars());
     /// let b = (0..).zip("ad".chars());
-    /// let it = a.merge_by(b, |x, y| Ord::cmp(&x.1, &y.1));
+    /// let it = a.merge_by(b, |x, y| x.1 <= y.1);
     /// itertools::assert_equal(it, vec![(0, 'a'), (0, 'b'), (1, 'c'), (1, 'd')]);
     /// ```
 
-    fn merge_by<J, F>(self, other: J, cmp: F) -> Merge<Self, J::IntoIter, F> where
+    fn merge_by<J, F>(self, other: J, is_first: F) -> MergeBy<Self, J::IntoIter, F> where
         Self: Sized,
         J: IntoIterator<Item=Self::Item>,
-        F: FnMut(&Self::Item, &Self::Item) -> Ordering
+        F: FnMut(&Self::Item, &Self::Item) -> bool
     {
-        Merge::new(self, other.into_iter(), cmp)
+        adaptors::merge_by_new(self, other.into_iter(), is_first)
     }
 
     /// Return an iterator adaptor that iterates over the cartesian product of

tests/tests.rs

@@ -372,7 +372,7 @@ fn merge_by() {
     let odd : Vec<(u32, &str)> = vec![(1, "hello"), (3, "world"), (5, "!")];
     let even = vec![(2, "foo"), (4, "bar"), (6, "baz")];
     let expected = vec![(1, "hello"), (2, "foo"), (3, "world"), (4, "bar"), (5, "!"), (6, "baz")];
-    let results = odd.iter().merge_by(even.iter(), |a, b|{ a.0.cmp(&b.0)});
+    let results = odd.iter().merge_by(even.iter(), |a, b| a.0 <= b.0);
     it::assert_equal(results, expected.iter());
 }
 
@@ -385,7 +385,7 @@ fn merge_by_btree() {
     let mut bt2 = BTreeMap::new();
     bt2.insert("foo", 2);
     bt2.insert("bar", 4);
-    let results = bt1.into_iter().merge_by(bt2.into_iter(), |a, b|{a.0.cmp(&b.0)});
+    let results = bt1.into_iter().merge_by(bt2.into_iter(), |a, b| a.0 <= b.0 );
     let expected = vec![("bar", 4), ("foo", 2), ("hello", 1), ("world", 3)];
     it::assert_equal(results, expected.into_iter());
 }