search

elmcraft / core-extra

Utility functions for an improved experience with elm/core
https://package.elm-lang.org/packages/elmcraft/core-extra/latest/
Other
22 stars 10 forks source link

Add warning message to List.Extra (suggested by elmcraft/core-extra#44) #46

Closed jmbromley closed 3 months ago

jmbromley commented 4 months ago

Add warning message for groupsOf... functions in the documentation in List.Extra as suggested in https://github.com/elmcraft/core-extra/issues/44#issuecomment-1968594245

gampleman commented 4 months ago

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?

Janiczek commented 4 months ago

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.

jmbromley commented 4 months ago

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 (https://github.com/elm-community/list-extra/issues/164#issuecomment-1140339068)

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 List.take from elm/core which only switches to a tail recursive implementation for lists larger than 1000 elements. This is a problem because the implementation of the groupsOf... family of functions is itself defined by recursion, so you can end up with the non-tail-recursive List.take being called a large number of times and blowing the stack (a real world example is at billstclair/elm-crypto-string#10).

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?

jmbromley commented 4 months ago

Okay, so I made a pull request that instead makes groupsOf... fully tail recursive (#47) as an alternative to this PR.

gampleman commented 3 months ago

Closed by #47