Add warning message to List.Extra (suggested by elmcraft/core-extra#44) #46
Conversation
Add warning message for `groupsOf...` functions in the documentation in `List.Extra` as suggested in elmcraft#44 (comment)
src/List/Extra.elm
Outdated
| @@ -62,6 +62,8 @@ module List.Extra exposing | |||
|
|
|||
| # Split to groups of given size | |||
|
|
|||
| > Note that (due to their usage of `List.take` from _elm/core_) the following functions are not always **strictly tail recursive**. For some large tasks it is possible you may run into runtime errors by exceeding the maximum call stack size. For help in this look [here](https://github.com/elmcraft/core-extra.git) and [here](https://github.com/elmcraft/core-extra.git). | |||
There was a problem hiding this comment.
Looks like this needs the actual links.
There was a problem hiding this comment.
Hmm, something went wrong with my copy/paste buffer. Fixed now!
|
This is fine as far as it goes. I wasn't there for the original issue, but is there a reason why the function wasn't made tail recursive to begin with? |
|
As the original author: no, there's no specific reason why I didn't write them in a tail-recursive way. Chalk that up to inexperience :) I'd prefer a tailrec version over a fast version. |
|
I actually made a PR that introduced separate tail recursive versions of the functions here: elm-community/list-extra#165 At the time @Chadtech thought it was too much of a corner case to risk a decrease in efficiency (elm-community/list-extra#164 (comment)) But now that I search the repository more carefully, I see that a few months prior to my PR there had been an effort to make it tail recursive (elm-community/list-extra#157) which was in fact merged, but did not actually fix the issue (see below). In that previous PR the discussion seemed to indicate that it was felt by many that correctness was preferred over efficiency, so maybe I should have pushed for it harder at the time. The reason the previous PR didn't actually make the functions tail recursive is because it still uses If you want to use a fully tail recursive version in all cases, it should be fairly straightforward to adapt my PR to do so - I'm happy to make a new PR if you'd like? |
This commit makes the `groupsOf...` family of functions fully tail recursive by forcing them to use the tail recursive version of List.take (normally List.take is only tail recursive for lists larger than 1000, but since the `groupsOf...` functions are themselves recursive this can result in potential call stack overflow from the successive accumulation of (up to) 1000-long non-recursive List.take calls during the recursion). This is an alternative to PR elmcraft#46 which would instead just add a note to the documentation warning users about the potential overflow.
|
Okay, so I made a pull request that instead makes |
Makes the `groupsOf...` family of functions fully tail recursive by forcing them to use the tail recursive version of List.take (normally List.take is only tail recursive for lists larger than 1000, but since the `groupsOf...` functions are themselves recursive this can result in potential call stack overflow from the successive accumulation of (up to) 1000-long non-recursive List.take calls during the recursion). This is an alternative to PR #46 which would instead just add a note to the documentation warning users about the potential overflow. Co-authored-by: Jakub Hampl <[email protected]>
|
Closed by #47 |
Add warning message for
groupsOf...functions in the documentation inList.Extraas suggested in #44 (comment)