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

Using orignal polymorphic compare operators #3053

Merged
merged 2 commits into from
May 16, 2019

Conversation

colin4124
Copy link
Contributor

Using orignal polymorphic compare operators in Caml instead of integer compare operators in Base.

Function find's interface is defined as polymorphic in .mli.

val find   : ('a, 'b) t -> 'a -> 'b option

But find's implement in .ml is inferred to val find : (int, 'a) t -> int -> 'a option

open Base

let find t key =
  List.find_map t.buckets.(hash_bucket key)
    ~f:(fun (key',data) -> if key' = key then Some data else None)

Because the type of the equality operator that’s open in Base is int -> int -> bool . So I use the original polymorphic operator in such way:

open Base

let find t key =
  List.find_map t.buckets.(hash_bucket key)
    ~f:(fun (key',data) -> if Caml.(key' = key) then Some data else None)

Using orignal polymorphic compare operators in Caml instead of integer compare operators in Base
@yminsky
Copy link
Member

yminsky commented Jan 22, 2019

That's a good find, but I don't really want to fix it this way. I'd rather modify the mli so that it takes an equality function as an argument, similar to how the List module works in Base.

https://github.com/janestreet/base/blob/master/src/list.mli#L432

Would you be up for adjusting the PR to do that?

@colin4124
Copy link
Contributor Author

@yminsky Thanks for your suggestion. Taking an equality function as an argument rather than using a polymorphic compare operator is more elegant.

I have adjusted, does it do that?

@XVilka
Copy link
Contributor

XVilka commented May 16, 2019

Ping? Looks good to merge.

Copy link
Member

@yminsky yminsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@yminsky yminsky merged commit ce0a4b4 into realworldocaml:master May 16, 2019
@samoht
Copy link
Contributor

samoht commented Jul 30, 2019

This needs to be reflected in the corresponding README too

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.

4 participants