Skip to content
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

List.filter_map #2185

Merged
merged 4 commits into from Dec 12, 2018

Conversation

Projects
None yet
6 participants
@trefis
Copy link
Contributor

commented Dec 7, 2018

No description provided.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

Thanks, this addition has been long overdue!

One question: Is such a function available in Batteries/Core/Containers, and if so, under which name(s)?

One remark: You made the function tail recursive. I think it becomes more and more difficult to justify than List.map, perhaps the most widely used function in List, is not tailrec, especially if all new additions, even those similar in spirit, are. I remember an old argument that the problem was not with List.map, but with users relying on list to hold large collections, but the argument is rather weak. Assuming the system cannot be fixed (e.g. with extensible stacks) to support non tailrec functions on long lists, one should probably aim at ensuring that all functions in List.map work on long lists (without degrading performance on small lists, hopefully).

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

All of them under the same name:

As for the tailrec discussion: I just copy pasted what was in Misc.Stdlib.List. If there is a general agreement that it should be implemented differently, I don't mind updating the PR. I personally do not care (well, apart from #181, I care about that).

@Chris00

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

should be implemented differently,

This should ideally be supported by benchmarks but how about to let it use "direct" recursion up to to a certain depth and, if that depth is exceeded, use your tail-recursive implementation on the remaining tail?

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

I don't think that any other function in the stdlib does this at the moment, and I don't plan to be the first person to add one like that.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

This should ideally be supported by benchmarks but how about to let it use "direct" recursion up to to a certain depth and, if that depth is exceeded, use your tail-recursive implementation on the remaining tail?

Yes, that's the usual way to get a function that works on long lists without the extra cost on small lists (and unrolling the loop a few times more than compensate the overhead of counting elements). One downside, though, is that such implementation might results in loosing inlining opportunities (with flambda) since the callback's body would be duplicated after inlining.

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 8, 2018

I believe that the implementation is correct, but I would still like to see tests for it.
(testsuite/tests/lib-list/test.ml has tests for list functions.)

@mookid

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

I don't think that any other function in the stdlib does this at the moment, and I don't plan to be the first person to add one like that.

init does that.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

init does that.

Indeed, I stand corrected.

Anyway: if we want to go with a "simple" version (i.e. either tail-rec or not tail-rec), I'd vote for the tail-rec one, because that's what's already used in the compiler and I don't want to:

  • have to implementations of the function lying around
  • think about whether these particular calls would be OK with a non-tail-rec version or not.
@Armael

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I think it's already very useful to have this added to the stdlib as-is -- improved implementations using unrolling can happen at a later time, and be done consistently on all the relevant functions at the same time.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

Ok, let's go with the tail-rec version. Please add a test, and this is good to merge I think.

trefis added some commits Dec 7, 2018

@trefis trefis force-pushed the trefis:filter_map branch from 81c2dd0 to ec694d6 Dec 11, 2018

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

Test added.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

@alainfrisch feel free to click the "squash and merge" button if you're happy with this :)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

I'm happy but I'd suffer @gasche's wrath if I accepted a Changes entry without proper author attribution.

@trefis

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

Ah! Indeed. I've updated the entry.

@alainfrisch alainfrisch merged commit 742c65d into ocaml:trunk Dec 12, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@nojb nojb referenced this pull request Feb 8, 2019

Closed

Add Array.map_{of,to}_list #2238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.