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

Use float for ordering, add Ordering module #149

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Jul 2, 2023

Naively converting JS code to rescript may result in unexpected and hard to track down bugs because the ints used for ordering overflows much easier than JS numbers.

This does two things:

  1. Uses floats instead of ints for ordering to avoid premature overflows.
  2. Adds an Ordering module to make comparison function code more readable, and more convenient to convert from ints.

@glennsl glennsl changed the title Use float for ordering Use float for ordering, add Ordering module Jul 2, 2023
Copy link
Collaborator

@zth zth 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 great to me! @cknitt ?

@cknitt
Copy link
Collaborator

cknitt commented Jul 3, 2023

I already suggested using floats once, see #74, but @cristianoc was not in favor of that at the time.

@zth
Copy link
Collaborator

zth commented Jul 3, 2023

Right, I forgot about that conversation it seems... 🙈

src/Core__Ordering.res Outdated Show resolved Hide resolved
@glennsl
Copy link
Contributor Author

glennsl commented Jul 3, 2023

I already suggested using floats once, see #74, but @cristianoc was not in favor of that at the time.

Huh, I'd forgotten about that too. Seems I've changed my mind. Maybe @cristianoc has too!

src/Core__Array.res Outdated Show resolved Hide resolved
@cristianoc
Copy link
Contributor

I already suggested using floats once, see #74, but @cristianoc was not in favor of that at the time.

Huh, I'd forgotten about that too. Seems I've changed my mind. Maybe @cristianoc has too!

I just made an observation not a recommendation.
Happy to go with whatever decision now that the relevant facts are known.

@zth zth merged commit aeee95d into rescript-association:main Jul 5, 2023
4 checks passed
@glennsl glennsl deleted the fix/use-float-for-ordering branch July 5, 2023 11:50
@jmagaram
Copy link
Contributor

Please see #150

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

5 participants