Add fromFoldable

[hdgarrood]

Refs #53. This implementation seems to work, but is not stack-safe (on my machine, on node.js, it fails with >= 5658 elements). Could arrays add a tailrec dependency, maybe?

[garyb]

If it doesn't cause a circular dependency adding a tailrec dependency sounds good to me 👍

[paf31]

Sounds good.

[hdgarrood]

This one is stack-safe. Unfortunately I couldn't work out how to do it with tailrec. I think I could do it with Trampoline from free but that would be quite a lot slower than this implementation, and would also cause circular dependencies, I think.

[hdgarrood]

This now fails because of this code:

exports.sortImpl = function (f) {
  return function (l) {
    /* jshint maxparams: 2 */
    return l.slice().sort(function (x, y) {
      return f(x)(y);
    });
  };
};

Removing the maxparams comment and removing maxparams from .jshintrc fixes it. I think this is probably the right approach, too; I don't think we gain anything from turning it on when we just turn it off again whenever we're required to write an uncurried function, and the tests should be catching any errors anyway.

[paf31]

👍 LGTM.

[sharkdp]

Neat trick 👍

I feel slightly uneasy about passing the effectful _push function to foldl. Is there any guarantee that Foldable instances will call the passed function exactly once for each 'element'? I can break the tests by adding a (admittedly supid) no-op to the Foldable instance for your Replicated type:

instance foldableReplicated :: Foldable Replicated where
  foldr f z (Replicated n x) = applyN n (f x) z
  foldl f z (Replicated n x) = let stupid = f z x
                               in applyN n (flip f x) z
  foldMap = foldMapDefaultR

Apart from that, it looks good to me, too! I'm eagerly awaiting fromFoldable.

[hdgarrood]

@sharkdp That's a good point. I was a bit uneasy about this too, but wasn't able to come up with an example that broke things. Maybe a better implementation would be something (conceptually) similar to foldr Cons Nil >>> listToArray then.

[hdgarrood]

This new implementation has what I would deem acceptable performance, ~100ms to run fromFoldable (replicate 100000 'a') on my machine. If people are worried about performance then they can use a more specialized alternative, anyway.

[hdgarrood]

Ok I actually benchmarked it instead of guessing

[sharkdp]

Looks good 👍.

Concerning the benchmark, a comparison to the _push implementation would be interesting.

[hdgarrood]

Good idea. As usual, my intuition was wrong:

[hdgarrood]

(I was expecting the stateful one to be quite a lot faster.)

[sharkdp]

Cool, thanks! I also expected a bigger difference..

[hdgarrood]

Unfortunately my tool is not very good at labeling the axes... but it looks like it is quite a lot faster for smaller arrays (<100,000 elements), at least. Anyway I'm fairly certain the viaList one is the right approach now.

[hdgarrood]

I've just squashed the commits, so the stateful implementation is gone now, but for anyone looking at this in the future, it was:

foreign import fromFoldableImpl :: forall f a.
  (forall b. (b -> a -> b) -> b -> f a -> b) -> f a -> Array a

fromFoldable :: forall f a. (Foldable f) => f a -> Array a
fromFoldable = fromFoldableImpl foldl

exports.fromFoldableImpl = function (foldl) {
  return function (xs) {
    var result = [];
    var push = function (x) { result.push(x); };
    var push_ = function () { return push; };
    foldl(push_)(null)(xs);
    return result;
  };
};

[garyb]

Last I knew using result.push was significantly slower than doing something like result[count++] = x, so that might have helped.

[ethul]

I agree with @garyb

https://jsperf.com/array-direct-assignment-vs-push

[garyb]

@hdgarrood could you rebase on the latest master please?

[tthdgarrood]

Done.

[garyb]

Thanks!