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

Make CCPersistentHashtbl.S.merge more general. #60

Merged
merged 2 commits into from Mar 14, 2016
Merged

Make CCPersistentHashtbl.S.merge more general. #60

merged 2 commits into from Mar 14, 2016

Conversation

johanneskloos
Copy link

This patch brings the merge function of CCPersistentHashtbl
in line with the merge functions of other maps
(Map, BatMap from batteries, CCWBTree). In particular, its signature
changes from the restrictive
merge: (key -> 'a option -> 'a option -> 'a option) -> 'a t -> 'a t -> 'a t
to a more general
merge: (key -> 'a option -> 'b option -> 'c option) -> 'a t -> 'b t -> 'c t

This patch brings the merge function of CCPersistentHashtbl
in line with the merge functions of other maps
(Map, BatMap from batteries). In particular, its signature
changes from the restrictive
  merge: (key -> 'a option -> 'a option -> 'a option) ->
    'a t -> 'a t -> 'a t
to a more general
  merge: (key -> 'a option -> 'b option -> 'c option) ->
    'a t -> 'b t -> 'c t
@c-cube
Copy link
Owner

c-cube commented Mar 9, 2016

Indeed, that's a good idea. Thanks for the patch, would you mind adding your name to AUTHORS.adoc before I merge?

As per maintainer request.
c-cube added a commit that referenced this pull request Mar 14, 2016
Make `CCPersistentHashtbl.S.merge` more general.
@c-cube c-cube merged commit 69e8f7a into c-cube:master Mar 14, 2016
@c-cube
Copy link
Owner

c-cube commented Mar 14, 2016

Thanks!

@rgrinberg
Copy link

Just a heads up, I think that core's interface is slightly stronger for this particular function. Rather than having 'a option -> 'b option core does

[ `Left of 'a
| `Right of 'b
| `Both of 'a * 'b ]

This is slightly superior because the two optional values being None is impossible (if I've interpreted what this function does correctly)

@c-cube
Copy link
Owner

c-cube commented Mar 14, 2016

Oh yes, I remember having seen that on #OCaml recently. That would be actually nice… But then we would need to have the same in CCMap (which would be a good thing).

@c-cube c-cube mentioned this pull request Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants