increase compatibility between BatMap, BatSplay and stdlib.Map #1008
Conversation
There was a problem hiding this comment.
Thanks for the code! I made a first pass of review.
I regret not insisting further to use our proper enumeration type on sets before, because the same issue is showing up again. If you are sure that you don't have the time or knowledge to provide better implementations of these functions, feel free to say so, and I can have a look, but it's actually not so hard to use them, so I think you could give it a try.
| @@ -79,6 +79,9 @@ module TestMap | |||
|
|
|||
| val equal : ('a -> 'a -> bool) -> 'a m -> 'a m -> bool | |||
|
|
|||
| (* true if add, remove, update_stdlib and filter support physical equality *) | |||
| val supports_phys_equality : bool | |||
There was a problem hiding this comment.
this is a nice trick to test this aspect of the Map module, thanks
|
Out of curiosity: what is the oldest ocaml version we still need to support with batteries-included? |
In general I think that the older version that it makes sense supporting is the OCaml version of the current Debian-stable release (or possibly oldstable if it's not too hard). Currently stable has 4.05 and oldstable has 4.02.3. Either of these would be fine. (On the other hand, using ppx rewriters can introduce a fair bit of maintenance churn/pain, so this is something to decide very carefully.) |
There was a problem hiding this comment.
I'm done reviewing the PR and I only had somewhat minor comments left. I believe that this is ready for merging; well done!
What kind of a git-history person are you? Would you like to rebase your commits before merging and merge some of them together, or leave things as-is, or that we squash all of them in a single commit?
| | None -> remove k m) | ||
| | None -> add k v m) | ||
| m1 | ||
| m2 |
There was a problem hiding this comment.
This implementation is not completely satisfying (it does not preserve the knowledge that we build the result in-order), but here the advice to use merge instead is not so convincing, because merge is okay but not that nice. (It performs an imperative traversal of two Enum, instead of working on the nicer, underlying 'a enumeration structures.) I propose to leave it as-is in the PR, and I maybe submit a subsequent PR to improve merge and then define union from it.
|
Considering git: If you prefer a "clean" history, feel free to squash the commits; I personally don't hold a strong opinion on the matter. |
There were several functions added to stdlib.Map, which are added in this PR.
Unfortunately, stdlib also added an update function with an incompatible type signature,
which is why this PR adds an update_stdlib function.
For 100% compatibility, something like
module IntMap = struct include BatMap.Int let update = update_stdlib endis necessary, see also the extended batteries_compattest.Also, for several functions the exception raised on empty maps was changed to
Not_foundto increase consistency and compatibility with stdlib; the exception raised was unspecified by the documentation for these functions anyways.