-
Notifications
You must be signed in to change notification settings - Fork 51
Combinators CPS reorganization #182
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
c3de0ba
to
dc9067c
Compare
I would like to merge something like this PR before we tag the next release for |
I'd be very grateful for your advice about this @natefaubion . This PR does not do everything which you suggested in your comments #154 (comment) and #177 (comment) and #154 (comment) but I believe it gives us the correct API, and is a strict improvement over the current situation. More optimizations can be done later. |
I think we should at least benchmark the fixed chainrRec. |
I think it's fine to keep using tailRecM, rather than just monadic recursion. For many of them they end up being equivalent due to the need for the |
dc9067c
to
209ef6e
Compare
Reorganize the Combinators to make sense with the CPS internals. These decisions were informed by benchmarking. * Export `many = List.manyRec` * Delete `sepBy`, replace with `sepByRec` * Delete `skipMany`, replace with `skipManyRec` * Delete `chainl`, replace with `chainlRec` * Delete `chainr`, replace with `chainrRec` (added `defer`) * Delete `manyTill`, replace with `manyTillRec`.
209ef6e
to
7e523ca
Compare
Adding a
|
Thanks for all the |
Oh wow, that drastic! Well, I guess any sane monad can be made Thank you for the update :-) |
Reorganize the Combinators to make sense with the CPS internals. These decisions were informed by benchmarking, see #177
many = List.manyRec
sepBy
, replace withsepByRec
skipMany
, replace withskipManyRec
chainl
, replace withchainlRec
chainr
, replace withchainrRec
(adddefer
Combinator variations #177 (comment))manyTill
, replace withmanyTillRec
.Resolves #177
Checklist: