Fold better; fix foldl1 bug

[treeowl]

• Index Arrays and SmallArrays eagerly.
• Fix foldl1 for Array, which was completely wrong.
• Make the structure of the Foldable instances for Array
  and SmallArray the same, taking what I thought was the best
  from each.

Fixes #86

[cartazio]

Look pretty solid. Could you explain what you mean by structure of the foldable instances ? The code looks clean and good, I’m just not sure if you mean formatting of instances or semantics :)

…

On Sun, Mar 18, 2018 at 1:54 AM David Feuer ***@***.***> wrote: - Index Arrays and SmallArrays eagerly. - Fix foldl1 for Array, which was completely wrong. - Make the structure of the Foldable instances for Array and SmallArray the same, taking what I thought was the best from each. Fixes #86 <#86> ------------------------------ You can view, comment on, or merge this pull request online at: #87 Commit Summary - Fold better; fix foldl1 bug File Changes - *M* Data/Primitive/Array.hs <https://github.com/haskell/primitive/pull/87/files#diff-0> (117) - *M* Data/Primitive/SmallArray.hs <https://github.com/haskell/primitive/pull/87/files#diff-1> (146) Patch Links: - https://github.com/haskell/primitive/pull/87.patch - https://github.com/haskell/primitive/pull/87.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#87>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwoeFLeOXtkCWUhAatBJKYp9u9xLxks5tffZ5gaJpZM4SvFRk> .

[treeowl]

Oh, I just meant the way the code is written. fix vs explicit recursion, the way the first/last element is handled in foldr1/foldl. I also removed the explicit toList implementations because they really just did the same as the default ones in the same way, which seemed a bit silly.

[RyanGlScott]

I haven't looked over everything in detail, but this looks good overall. As you note in #89, it would be nice to have a regression test for foldl'. We could:

1. Wait until the test suite is overhauled, and then rebase this PR on top of that
2. Merge this now, and ask that you write a regression test later :)

Which option would you prefer?

[RyanGlScott]

Out of curiosity, does this generate better code than the default implementation for sum (which is something like getSum . foldMap Sum)?

[treeowl]

I think we can merge this now. I don't want to have to keep rebasing the traversal stuff over it.

…

On Mar 19, 2018 11:20 AM, "Ryan Scott" ***@***.***> wrote: ***@***.**** commented on this pull request. I haven't looked over everything in detail, but this looks good overall. As you note in #89 <#89>, it would be nice to have a regression test for foldl'. We could: 1. Wait until the test suite is overhauled, and then rebase this PR on top of that 2. Merge this now, and ask that you write a regression test later :) Which option would you prefer? ------------------------------ In Data/Primitive/SmallArray.hs <#87 (comment)>: > + sz = sizeofSmallArray ary + go i !e + | i == sz = e + | (# x #) <- indexSmallArray## ary i + = go (i+1) (max e x) + {-# INLINE maximum #-} + minimum ary | sz == 0 = die "minimum" "Empty SmallArray" + | (# frst #) <- indexSmallArray## ary 0 + = go 1 frst + where sz = sizeofSmallArray ary + go i !e + | i == sz = e + | (# x #) <- indexSmallArray## ary i + = go (i+1) (min e x) + {-# INLINE minimum #-} + sum = foldl' (+) 0 Out of curiosity, does this generate better code than the default implementation for sum (which is something like getSum . foldMap Sum)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_c6pllCoVhB6JlvcvaWHGWiYNT7Pks5tf8zSgaJpZM4SvFRk> .

[treeowl]

Thanks.

…

On Mar 19, 2018 11:55 AM, "Ryan Scott" ***@***.***> wrote: Merged #87 <#87>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_d0F0mAQuQAMvsENgCNltbp9EHA9ks5tf9TbgaJpZM4SvFRk> .

[andrewthad]

For posterity's sake, I'll point out that foldl1 is incorrect in this PR but has been fixed in #99.

[treeowl]

I'm surprised. I did test it manually with the list monoid. Maybe I did something stupid afterwards?

…

On Fri, Mar 23, 2018, 10:32 AM Andrew Martin ***@***.***> wrote: For posterity's sake, I'll point out that foldl1 is incorrect in this PR but has been fixed in #99 <#99>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#87 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_dLgGhvDQnr2z4_fHjkFxlFM3ZOQks5thQdqgaJpZM4SvFRk> .

[andrewthad]

Maybe you accidentally picked a list that was the reversal of itself. Or maybe you accidentally revert part of your changes before committing. Either way, not a big deal now that we have decent tests.