Foldable/Reducible intercalate update

[peterneyens]

Follow up on #1520, after @johnynek 's comment.

This should reduce the complexity for intercalate from O(N^2) to O(N).
I also overrode toList in some Foldable instances.

[peterneyens]

I could probably skip the unnecessary reduce step for one.

  toNonEmptyList(fa) match {
    case NonEmptyList(hd, Nil) => hd
    case NonEmptyList(hd, tl) => 
      Reducible[NonEmptyList].reduce(NonEmptyList(hd, a :: intersperseList(tl, a)))
  }

[johnynek]

👍

[codecov-io]

Current coverage is 92.37% (diff: 100%)

Merging #1522 into master will decrease coverage by 0.03%

@@             master      #1522   diff @@
==========================================
  Files           246        246          
  Lines          3743       3763    +20   
  Methods        3622       3637    +15   
  Messages          0          0          
  Branches        121        126     +5   
==========================================
+ Hits           3459       3476    +17   
- Misses          284        287     +3   
  Partials          0          0          

Powered by Codecov. Last update fd15e07...b723dea

[travisbrown]

👍

[johnynek]

Actually it looks like Foldable.combineAll is just an alias for fold which does not call Monoid.combineAll. I suggest we make Foldable.combineAll be Monoid.combineAll(fa.toList)

[peterneyens]

Updated to use the combineAll from Monoid.

It seems we were already using Semigroup.combineAllOption to implement Reducible#reduce for NonEmptyVector, so I went ahead and did the same for NonEmptyList and overrode the fold method of the Foldable instances of List, Vector, Set, Stream and Map. (Hmm, this might have deserved its own PR.)

[johnynek]

👍

[kailuowang]

I probably missed something but I am curious what gain we get from overriding this? I mean the
In Monoid.combineAll it's

as.foldLeft(empty)(combine)

In Foldable.fold it's

 foldLeft(fa, A.empty) { (acc, a) =>
    A.combine(acc, a)
 }

[peterneyens]

I was confused about that too.

When you run the MapMonoidBench, you can see that the first using Monoid.combineAll is considerably faster than the second.

That's because Monoid.combineAll is overridden in some instances, eg Map and more general Iterable.

[kailuowang]

got it. that's what I suspected, but I only checked the List instance.

[kailuowang]

👍