Skip to content

Fix overflows in randomInt #12

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

Merged
merged 1 commit into from
Aug 2, 2015
Merged

Fix overflows in randomInt #12

merged 1 commit into from
Aug 2, 2015

Conversation

hdgarrood
Copy link
Contributor

Fixes #9

Fix overflows by turning everything into a Number first, and then converting it back at the end.

I decided not to bother with the export list yet because I thought you might want safeFromNumber to go into purescript-integers instead, perhaps with a different name. (truncate?) What do you think?


-- | Safely convert a Number into an Int, by first flooring and then clamping
-- to the valid integer range.
safeFromNumber :: Number -> Int
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be exported, right? Can it go into a where clause?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should read your comments first :)

Yeah, let's put this into integers as truncate. Although, I guess it depends what we decide to do about bounds on integers for different backends.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shouldn't, but I didn't do that yet becaues I thought you might want to put this function into integers instead. I can imagine it being useful in other contexts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, just saw your second comment!

Regardless of what we do about different backends, top :: Int should still work on any backend, right? Perhaps Prelude can get the maximum and minimum values for the Bounded Int instance from FFI files, so that we can make them differ across backends?

@garyb
Copy link
Member

garyb commented Jul 26, 2015

What is this actually fixing? Since high is an Int, isn't it already safe from overflow?

@hdgarrood
Copy link
Contributor Author

If you ask for randomInt bottom top, then the current code will do:

toNumber (high - low + one) * n + toNumber low
== toNumber (top - bottom + one) * n + toNumber bottom
== toNumber 0 * n + toNumber bottom
== 0 + bottom
== bottom

so randomInt bottom top always evaluates to bottom.

@garyb
Copy link
Member

garyb commented Jul 26, 2015

Ah ok :)

@hdgarrood
Copy link
Contributor Author

Now that integers has been updated and released, I think this is good to go.

garyb added a commit that referenced this pull request Aug 2, 2015
@garyb garyb merged commit 6e0ca41 into purescript:master Aug 2, 2015
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.

3 participants