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

Array.sort: compare function return type #74

Closed
cknitt opened this issue Feb 26, 2023 · 6 comments
Closed

Array.sort: compare function return type #74

cknitt opened this issue Feb 26, 2023 · 6 comments

Comments

@cknitt
Copy link
Collaborator

cknitt commented Feb 26, 2023

Currently, the compare function for Array.sort/Array.sortInPlace is typed as

('a, 'a) => int

I do realize that this is also the signature used in OCaml's compare functions, but I wonder if the return type shouldn't be float instead for the ReScript/JS world. Looking at the MDN documentation for Array.sort, there is no need for the return type to be an integer, it is just about > 0, < 0 or === 0.

Using float instead of int would be more generic and allow us e.g. to use (a, b) => a -. b as a compare function for floats.

What do you think?

@namenu
Copy link
Contributor

namenu commented Feb 26, 2023

float can be #Nan, #Inf, or negative zero so I think it would be safe to represent in int.
In my Chrome, returning NaN in the compare function doesn't crashes with undesirable output.

@glennsl
Copy link
Contributor

glennsl commented Feb 26, 2023

Using float instead of int would be more generic and allow us e.g. to use (a, b) => a -. b as a compare function for floats.

I don't see how it's really more generic. int is not a subtype of float. They're entirely distinct types that have to be converted whether it's to or from the other type. And while the domain of float does include the entire domain of int, the additional domain of float doesn't seem to be significant.

Also, having to deal with NaN means you cannot assume that there are only three possibilities: greater than, equal, and less than. NaN is neither. All of these expressions evaluate to false:

NaN > 0 // false
NaN < 0 // false
NaN == 0 // false
NaN == NaN // false

From a perspective of both simplicity and practicality it seems to me that it would be better to stick with int.

@cknitt
Copy link
Collaborator Author

cknitt commented Feb 26, 2023

I don't see how it's really more generic. int is not a subtype of float. They're entirely distinct types that have to be converted whether it's to or from the other type. And while the domain of float does include the entire domain of int, the additional domain of float doesn't seem to be significant.

What I meant is that, with a float result, both (a, b) => a -. b (for floats) and (a, b) => Float.fromInt(a - b) (for ints) make sense as comparators, whereas with an int result, (a, b) => Float.toInt(a -. b) does not (but of course a float compare function can be implemented that returns -1, 0 or 1 for the respective case, so you are right, in that sense it is not more generic, just more convenient).

float can be #Nan, #Inf, or negative zero so I think it would be safe to represent in int.

Is it guaranteed that an int can never be NaN? At least I can get Infinity with e.g. 10->Math.Int.pow(~exp=1000). BTW, does this mean that Math.Int.pow should be changed to return option?

@cristianoc
Copy link
Contributor

Float is not as first class as int, in terms of learning curve and primitives. One ends up in the dotted variants of operations etc.

@zth
Copy link
Collaborator

zth commented Feb 27, 2023

Good points. Sounds to me like float wouldn't give that much benefit. What do you say @cknitt ?

@cknitt
Copy link
Collaborator Author

cknitt commented Feb 27, 2023

Yes, good points, thanks for the answers. Will close this issue.

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

No branches or pull requests

5 participants