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

PR#6279: add Set.map to stdlib #553

Merged
merged 1 commit into from May 2, 2016

Conversation

Projects
None yet
6 participants
@gasche
Member

gasche commented Apr 18, 2016

Show outdated Hide outdated stdlib/set.mli
Show outdated Hide outdated stdlib/set.mli
Show outdated Hide outdated stdlib/set.mli

@damiendoligez damiendoligez added this to the 4.04 milestone Apr 19, 2016

Show outdated Hide outdated stdlib/moreLabels.mli
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Apr 25, 2016

Contributor
  • I would either make it explicit that the function must be the physical identity on mapped elements, or test with Ord.comparerather than ==.
  • What about providing filter_map (with a function `'a -> 'a option') as well?
Contributor

alainfrisch commented Apr 25, 2016

  • I would either make it explicit that the function must be the physical identity on mapped elements, or test with Ord.comparerather than ==.
  • What about providing filter_map (with a function `'a -> 'a option') as well?
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 25, 2016

Member

Using Ord.compare would be wrong if map intentionally update parts of the structure that are not taken into account by the comparison function (for example the values have a unique identifier, and compare uses them only, but the function updates another field). I'll update the specification.

Member

gasche commented Apr 25, 2016

Using Ord.compare would be wrong if map intentionally update parts of the structure that are not taken into account by the comparison function (for example the values have a unique identifier, and compare uses them only, but the function updates another field). I'll update the specification.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 25, 2016

Member

I updated the branch.

Member

gasche commented Apr 25, 2016

I updated the branch.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Apr 25, 2016

Contributor

If we're going to add filter_map here it would be nice to add it to the rest of the appropriate stdlib modules as well.

Contributor

hcarty commented Apr 25, 2016

If we're going to add filter_map here it would be nice to add it to the rest of the appropriate stdlib modules as well.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Apr 25, 2016

Member

I think I'm not going to work on filter_map as part of this pull-request.

Member

gasche commented Apr 25, 2016

I think I'm not going to work on filter_map as part of this pull-request.

@alainfrisch alainfrisch merged commit 3fe0cb1 into ocaml:trunk May 2, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment