generalize type of Data.Set.unions (from List to Foldable)? #520
Comments
|
I doubt very much that we need `foldlStrict` anymore. Do you want to open a
PR to get rid of it, make everything use `foldl'` instead, and generalize
the type of `unions` in all four modules as you suggest?
…On Tue, Jan 30, 2018 at 3:40 AM, jwaldmann ***@***.***> wrote:
We have unions :: Ord a => [Set a] -> Set a. Haddocks say that unions ==
foldl union empty
so I'd expect it to have type (Foldable t, Ord a) => t (S.Set a) -> S.Set
a, the type that would be inferred when I literally use the given
definition.
Yes, I know that it's S.foldl, not Data.Foldable.foldl. Why? If there is
a reason, then it would be good to have it documented (visibly). But instance
Foldable Set where foldl = foldl, so they *are* equivalent already? The
actual source has unions = foldlStrict union empty, so the docs are not
telling the full truth.
There is a comment at https://hackage.haskell.org/
package/containers-0.5.11.0/docs/src/Utils.Containers.
Internal.StrictFold.html#foldlStrict but this is hard to find? (source is
there but its rendered haddock is not, since this is from other-modules)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#520>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_fTB5CrV1nxfjd_-5R-gbMGmVVwkks5tPtVvgaJpZM4Rx8nB>
.
|
|
I'll see to this later this week. |
|
Side note: the function (with the type) that I wanted is already there - it's |
|
I made the PR since you said so, and to have a basis for discussion. Before actually adopting this -
|
|
This doesn't change the public API. The `foldlStrict` business showed up
long ago when foldl' didn't fuse and wasn't even marked INLINE. I will be
pretty shocked if anything goes wrong with the benchmarks, but you can run
them if you like. Even if there are small regressions, I'll likely tolerate
them to clean things up.
|
|
I meant the type of |
|
Oh, well, that's true. It's very rare for code to rely on the consumer to
determine which Foldable it's using, and I'm pretty sure we're not
sacrificing any realistic optimization opportunities by generalizing this.
But someone might object to the ascending/descending versions not being
generalized, and those are less clear-cut. If you feel more comfortable,
you can change the types back on this PR and propose the change on the
list. I don't feel too strongly about this one.
…On Feb 2, 2018 4:54 PM, "jwaldmann" ***@***.***> wrote:
I meant the type of unions (now has Foldable)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#520 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_aVBcX2AKRDkjDiOl0yKkzsgUN8Qks5tQ4P9gaJpZM4Rx8nB>
.
|
treeowl
pushed a commit
that referenced
this issue
Mar 9, 2018
|
Closed by #524. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We have
unions :: Ord a => [Set a] -> Set a. Haddocks say thatunions == foldl union emptyso I'd expect it to have type
(Foldable t, Ord a) => t (S.Set a) -> S.Set a, the type that would be inferred when I literally use the given definition.Yes, I know that it's
S.foldl, notData.Foldable.foldl. Why? If there is a reason, then it would be good to have it documented (visibly). Butinstance Foldable Set where foldl = foldl, so they are equivalent already? The actual source hasunions = foldlStrict union empty, so the docs are not telling the full truth.There is a comment at https://hackage.haskell.org/package/containers-0.5.11.0/docs/src/Utils.Containers.Internal.StrictFold.html#foldlStrict but this is hard to find? (source is there but its rendered haddock is not, since this is from other-modules)
The text was updated successfully, but these errors were encountered: