-
Notifications
You must be signed in to change notification settings - Fork 40
Make List.zipWith as lazy as expected #492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
972fb7d to
08a229a
Compare
tbagrel1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think zipWithK is very close to a dual-fold/bi-fold/zip-fold (not sure if there is a standard name), if we fuse the f and cons functions into one of type a -> b -> r -> r. Maybe it would make for a more general/intuitive function?
| where | ||
| go :: [a] %1 -> [b] %1 -> r | ||
| go [] [] = nil | ||
| go (a : as) [] = lefta a as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not entierely clear why the left{a,b} functions are taking head and tail as separate arguments. I guess the motivation is to make a NonEmpty list in zipWith' without having to call NonEmpty.fromList?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could say that. Or equivalently, that lefta has all the available information. Or equivalently, that it makes invalid states unrepresentable. It's the natural type for this function don't you think?
| go [] (b : bs) = leftb b bs | ||
| go (a : as) (b : bs) = cons (f a b) (go as bs) | ||
|
|
||
| zipWith3 :: forall a b c d. (Consumable a, Consumable b, Consumable c) => (a %1 -> b %1 -> c %1 -> d) -> [a] %1 -> [b] %1 -> [c] %1 -> [d] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does zipWith3 doesn't get the same treatment with a zipWithK3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there's no zipWith3' with rests. And that, in turn, is because we didn't give it a type (using Either is a bit of a shortcut for zipWith', maybe it was a bad shortcut).
We could make a zipWith3K if we decide to export it.
| zipWithk f (:) [] consume2 consume2 | ||
| where | ||
| consume2 :: forall x y z. (Consumable x, Consumable y) => x %1 -> y %1 -> [z] | ||
| consume2 x y = x `lseq` y `lseq` [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In zipWith3 you do (x, y) `lseq` [] instead, any reason to prefer one or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes much difference after optimisation. In principle, even without optimisation x `lseq` y `lseq` [] is going to be one fewer allocation.
aspiwack
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really thought I replied at the time. Got consumed by silly tasks in the meantime, let me address the comments quickly today and then merge.
| zipWithk f (:) [] consume2 consume2 | ||
| where | ||
| consume2 :: forall x y z. (Consumable x, Consumable y) => x %1 -> y %1 -> [z] | ||
| consume2 x y = x `lseq` y `lseq` [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes much difference after optimisation. In principle, even without optimisation x `lseq` y `lseq` [] is going to be one fewer allocation.
| where | ||
| go :: [a] %1 -> [b] %1 -> r | ||
| go [] [] = nil | ||
| go (a : as) [] = lefta a as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could say that. Or equivalently, that lefta has all the available information. Or equivalently, that it makes invalid states unrepresentable. It's the natural type for this function don't you think?
| go [] (b : bs) = leftb b bs | ||
| go (a : as) (b : bs) = cons (f a b) (go as bs) | ||
|
|
||
| zipWith3 :: forall a b c d. (Consumable a, Consumable b, Consumable c) => (a %1 -> b %1 -> c %1 -> d) -> [a] %1 -> [b] %1 -> [c] %1 -> [d] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there's no zipWith3' with rests. And that, in turn, is because we didn't give it a type (using Either is a bit of a shortcut for zipWith', maybe it was a bad shortcut).
We could make a zipWith3K if we decide to export it.
I'd missed this. Yes, it is a |
I was wrong, the zip-fold thing yields the very same code. Excellent idea! There is no such function in the Hoogle database at all. I'll name it |
69a4215 to
b73d0f4
Compare
|
Ok. I'll merge when green. I did export the I didn't write a I'll cut a minor release with this fix on Tuesday, so we have until then to figure out if there's a scalable way to represent leftovers wider |
b73d0f4 to
cab9d3e
Compare
|
Thanks for taking time to address my comments! LGTM |
Fixes #491