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

Dynarray.{equal, compare} #13144

Merged
merged 1 commit into from
May 16, 2024
Merged

Dynarray.{equal, compare} #13144

merged 1 commit into from
May 16, 2024

Conversation

gasche
Copy link
Member

@gasche gasche commented May 3, 2024

This was suggested by @OlivierNicole in #12885 (comment) .

Having comparison functions for Dynarray is useful because they do not have a canonical representation: two dynarrays may have the same elements, but yet have (=) return false or Stdlib.compare return non-0 due to having a different capacity.

I reused and adapted the docstring of List.equal and Seq.compare.

Comparing of different-length arrays

For compare there is a choice between:

  1. what wikipedia calls the lexicographic order: [10] > [1; 2] because 10 > 1
  2. what wikipedia calls the "shortlex" order: [10] < [1; 2] because the first list is shorter (before comparing the elements)

Using Stdlib.compare (Dynarray.to_list a) (Dynarray.to_list b) would result in lexicographic order (1), and Stdlib.compare (Dynarray.to_array a) (Dynarray.to_array b) would result in shortlex order (2). (In other words, "doing the same as other data structures" does not force us to choose one or the other.)

I went for the shortlex order which is slightly faster.

@gasche gasche added the stdlib label May 3, 2024
stdlib/dynarray.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@dbuenzli dbuenzli left a comment

Choose a reason for hiding this comment

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

I just left bikesheedding comments on the docstrings.

stdlib/dynarray.mli Outdated Show resolved Hide resolved
stdlib/dynarray.mli Outdated Show resolved Hide resolved
stdlib/dynarray.mli Outdated Show resolved Hide resolved
@gasche gasche force-pushed the dynarray-compare branch 2 times, most recently from 45b2f89 to f562733 Compare May 4, 2024 08:10
@gasche
Copy link
Member Author

gasche commented May 4, 2024

Thanks both, I took your comments into account.

@yallop
Copy link
Member

yallop commented May 4, 2024

Having comparison functions for Dynarray is useful because they do not have a canonical representation: two dynarrays may have the same elements, but yet have (=) return false or Stdlib.compare return non-0 due to having a different capacity.

Extending this reasoning, we should have comparison functions for other "container" types in the standard library, too, because their elements might not have canonical representations. For example, Array.equal is also useful, because (=) might return false for an int Dynarray.t Array.t comparison where corresponding Dynarray.t elements have the same int elements but different capacities.

@gasche
Copy link
Member Author

gasche commented May 4, 2024

I don´t disagree and in fact I was surprised to find that Array does not have those functions. (We have equal functions for base types thanks to #84.) But I would prefer to restrict the scope of the present PR to Dynarray comparisons only to avoid getting stuck.

Copy link
Contributor

@OlivierNicole OlivierNicole left a comment

Choose a reason for hiding this comment

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

This looks good to me.

let compare cmp a1 a2 =
let Pack {arr = arr1; length = length; dummy = dum1} = a1 in
let Pack {arr = arr2; length = len2; dummy = dum2} = a2 in
if length <> len2 then length - len2
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart!

Copy link
Member Author

Choose a reason for hiding this comment

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

(... and only correct because we know that they are both non-negative, I think.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, insofar as you consider that a dynarray of length −12 is lesser than a dynarray of length −2, it is correct!

@gasche gasche merged commit 7d65b50 into ocaml:trunk May 16, 2024
13 of 18 checks passed
@shindere
Copy link
Contributor

shindere commented May 17, 2024 via email

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

Successfully merging this pull request may close these issues.

None yet

5 participants