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

Fixes Basics.truncate for values larger than 32 bit ints #842

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Grannath

Grannath commented Mar 7, 2017

The current native JavaScript implementation uses the n | 0 bit-wise operation to truncate the value. This is restricted by the definition of bit-wise operations. They are defined to operate on 32 bit signed integers.

In general, JavaScript allows 64 bit floating point numbers. With these, integers can be represented up to 2^53 - 1 aka Number.MAX_SAFE_INTEGER. By using a bit-wise operation, the usable range of Int in Elm is restricted artificially.

The implementation is replaced here with Math.ceil and Math.floor depending on the sign. Testing in a REPL now successfully evaluates e.g. truncate <| 2.45 * (2 ^ 33), which would have been impossible before.

Such a restriction is a) unnecessary and b) unexpected. I highly suggest removing it.

Johannes Zick (extern)
Fixes Basics.truncate
The old implementation used a bit-wise operation. While it might be slightly
faster, it also fails on values larger than ~2^31. Replacing it with Math.ceil
or Math.floor depending on the arguments sign sidesteps this problem.
@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Mar 7, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Mar 7, 2017

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@witoldsz

This comment has been minimized.

Show comment
Hide comment
@witoldsz

witoldsz Apr 20, 2017

@evancz could you consider merging it "as is"? Today I had a nasty bug because of the return n | 0 implementation of truncate (the doc says nothing about cutting the number to 32 bits). The #721 umbrella is a nice idea, but this single bug could be fixed just now, what would you say?

witoldsz commented Apr 20, 2017

@evancz could you consider merging it "as is"? Today I had a nasty bug because of the return n | 0 implementation of truncate (the doc says nothing about cutting the number to 32 bits). The #721 umbrella is a nice idea, but this single bug could be fixed just now, what would you say?

@harfangk

This comment has been minimized.

Show comment
Hide comment
@harfangk

harfangk Jan 22, 2018

I ran into this issue myself today. I hope this would get merged soon!
By the way, there's also Math.trunc() function in ES5, which corresponds to this function, but of course it's supported on relatively newer browsers.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/trunc

harfangk commented Jan 22, 2018

I ran into this issue myself today. I hope this would get merged soon!
By the way, there's also Math.trunc() function in ES5, which corresponds to this function, but of course it's supported on relatively newer browsers.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/trunc

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