Skip to content

Add [Float.]Array.{equal.compare} - #13836

Merged
nojb merged 7 commits into
ocaml:trunkfrom
dbuenzli:array-compare
Mar 13, 2025
Merged

Add [Float.]Array.{equal.compare}#13836
nojb merged 7 commits into
ocaml:trunkfrom
dbuenzli:array-compare

Conversation

@dbuenzli

Copy link
Copy Markdown
Contributor

As the title says. Two notes for reviewers:

  1. It seems that test_array.ml uses snapshot testing. I don't think it's that great for testing this kind of library code. So you may find the test structure a bit at odds (but if an assert fails the empty expectation fails).

  2. There's a bit of tricky stuff around compare whose naked incarnation is used by Array.mem (and Float.Array.mem which uses Float.compare) which is bellow the compare addition. Since there's currently no dependency on Stdlib in array.ml I prefered not to use Stdlib.compare but rename it locally. If that's a dependency that is fine to introduce I can change that. Looking forward to Add a Repr module to the standard library #13755

P.S. Contributing functions to Array is painfull, between the label madness and Float.Array one spends most time bureaucratizing around and c&p code :-(

@lthls

lthls commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

Since there's currently no dependency on Stdlib in array.ml I prefered not to use Stdlib.compare but rename it locally

Technically, there is: Stdlib is opened implictly, and things like invalid_arg and compare already introduce a dependency on Stdlib. But it's possible that using Stdlib.compare would make ocamldep see the dependency, which may or may not be an issue, so being conservative fells justified.

@dbuenzli

Copy link
Copy Markdown
Contributor Author

But it's possible that using Stdlib.compare would make ocamldep see the dependency,

Yes that's what happend, a dependency on stdlib.cmi was added on stdlib__Array.* so I backed off (note however that stdlib_Float.* already has it, so I used it there).

@dra27 dra27 added the stdlib label Mar 5, 2025

@nojb nojb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I would avoid the exception.

A second approval from a core dev is needed to move forward with the PR.

Comment thread stdlib/array.ml Outdated
Comment thread stdlib/array.ml Outdated
Comment thread stdlib/array.ml Outdated
Comment thread stdlib/float.ml
@dbuenzli

dbuenzli commented Mar 6, 2025

Copy link
Copy Markdown
Contributor Author

Thanks @nojb for the review. Your comments have been addressed.

Comment thread stdlib/array.ml Outdated
@dbuenzli

dbuenzli commented Mar 6, 2025

Copy link
Copy Markdown
Contributor Author

Not sure what happend with the CI of MSVC / clang-cl 64 bits:

List of failed tests:
    tests/compaction/test_compact_manydomains.ml

@nojb nojb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, modulo nits below

Comment thread Changes Outdated
Comment thread stdlib/float.mli Outdated

@gasche gasche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks nice to me as well.

Note: adding Array.compare may break code in the ecosystem that uses stdlib compare in an Array.(...) context, but this breakage would result in an type error and can be fixed in a backward-compatible way.

Comment thread testsuite/tests/lib-array/test_array.ml
@dbuenzli

dbuenzli commented Mar 9, 2025

Copy link
Copy Markdown
Contributor Author

Note: adding Array.compare may break code in the ecosystem that uses stdlib compare in an Array.(...)

At least on the sherlocode corpus of OCaml code and assuming this happens on a single line, this doesn't seem to happen.

@dbuenzli

Copy link
Copy Markdown
Contributor Author

I think this could be merged.

@nojb
nojb merged commit 2038290 into ocaml:trunk Mar 13, 2025
@nojb

nojb commented Mar 13, 2025

Copy link
Copy Markdown
Contributor

Thanks for the ping!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants