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

Implemented find_opt in Map.S interface #941

Merged
merged 2 commits into from
Feb 5, 2020
Merged

Implemented find_opt in Map.S interface #941

merged 2 commits into from
Feb 5, 2020

Conversation

nicoTolly
Copy link
Contributor

@nicoTolly nicoTolly commented Feb 4, 2020

Related to this issue : #940

This Pull request adds a find_opt function to the Map.S interface. It appears that there was already a find_option implemented in Map.Concrete so I just used that. Another implementation was needed in BatSplay.ml so I added one. Also note that a function with the same type is implemented in Map.S.Exceptionless. Maybe we should do some factorization, but for now I changed the code as little as possible.
There was a discussion on the issue on which syntax should be used (about whether or not use the match... exception syntax). I'm still not clear on the subject, so I'm waiting for your comments.

@UnixJunkie
Copy link
Member

Thanks a lot for sending a PR.

Can you replace the implementation by this:

let find_opt k m =
  try Some (find k m)
  with Not_found -> None

Because it will compile whatever the OCaml version.

Also, the unit tests are usually a few line as comments in the same file.
You don't need to change test_map.ml.

Removed test in test_map, changed batSplay.ml implementation
@nicoTolly
Copy link
Contributor Author

Done. As an aside, it seems that make test does not automatically recompile every changed files (at some point, make test failed, but make clean/make test succeeded without changing the code). Is it a known problem ?

@UnixJunkie
Copy link
Member

Thanks a lot, perfect!

@UnixJunkie UnixJunkie merged commit 01e81d6 into ocaml-batteries-team:master Feb 5, 2020
@UnixJunkie
Copy link
Member

I think I noticed this recompile problem. But, I am too lazy to dig into a hairy Makefile.
Also, we should use dune and get rid of the current build system in the long run, which
also adds to my low motivation...

@nicoTolly nicoTolly deleted the find_opt branch February 5, 2020 09:41
@rixed
Copy link
Contributor

rixed commented Feb 5, 2020

FTR: these days dependencies are not handled with make but with ocamlbuild

@rixed
Copy link
Contributor

rixed commented Feb 17, 2020

find_opt has been added only in v4.05 of ocaml stdlib whereas batteries claim to support ocaml >= 4.00, therefore compilation is broken for those older compiler.
Anyway, the idea of the Ord module is to use the provided comparison function though, so Stdlib.find_opt won't cut it.

Fixed in v3.0.0 I think.

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