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 Math.trunc in Basics.truncate #591

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@pisys
Contributor

pisys commented May 10, 2016

Native Basics.truncate is implemented using a bitwise operator. The problem is that Javascript transforms the bit length from 64 to 32 bit for any number involved in bitwise operation.
ES6 offers Math.trunc which offers the same functionality but keeps the bit length. We should use this instead.

Use Math.trunc in Basics.truncate
Native Basics.truncate is implemented using a bitwise operator. Javascript transforms the bit length from 64 to 32 bit for any number involved in bitwise operation. 
ES6 offers Math.trunc which keeps the bit length.
@justinmimbs

This comment has been minimized.

Show comment
Hide comment
@justinmimbs

justinmimbs Jul 7, 2016

Right on. But since Math.trunc isn't universally supported yet, may I suggest using n < 0 ? Math.ceil(n) : Math.floor(n) in the meantime?

justinmimbs commented Jul 7, 2016

Right on. But since Math.trunc isn't universally supported yet, may I suggest using n < 0 ? Math.ceil(n) : Math.floor(n) in the meantime?

@pisys

This comment has been minimized.

Show comment
Hide comment
@pisys

pisys Jul 8, 2016

Contributor

Yes, thanks for your suggestion. I made another commit.

Contributor

pisys commented Jul 8, 2016

Yes, thanks for your suggestion. I made another commit.

@lukewestby lukewestby added the bug label Aug 20, 2016

@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby Aug 20, 2016

Member

Like @pisys mentioned, the current truncate only works for numbers in (-858993459, 858993459). I benchmarked @pisys's change against a couple other methods that don't exhibit this issue and this method seems to be pretty much on par with x - x % 1 and about 10x faster than parseInt(). I think it's good to merge in.

http://jsbin.com/zazitofaco/edit?js,console

Member

lukewestby commented Aug 20, 2016

Like @pisys mentioned, the current truncate only works for numbers in (-858993459, 858993459). I benchmarked @pisys's change against a couple other methods that don't exhibit this issue and this method seems to be pretty much on par with x - x % 1 and about 10x faster than parseInt(). I think it's good to merge in.

http://jsbin.com/zazitofaco/edit?js,console

@evancz evancz referenced this pull request Sep 22, 2016

Open

Math and numbers #721

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Sep 22, 2016

Member

I'm putting this in #721 because it will change how people's programs work.

As far as I know, JS does not have 64 bit integers. So when converting from float to int, why would we expect to get anything besides a 32 bit integer?

Member

evancz commented Sep 22, 2016

I'm putting this in #721 because it will change how people's programs work.

As far as I know, JS does not have 64 bit integers. So when converting from float to int, why would we expect to get anything besides a 32 bit integer?

@evancz evancz closed this Sep 22, 2016

@yjkogan

This comment has been minimized.

Show comment
Hide comment
@yjkogan

yjkogan Sep 22, 2016

Yeah my understanding is that all numbers in javascript are doubles

yjkogan commented Sep 22, 2016

Yeah my understanding is that all numbers in javascript are doubles

@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby Sep 22, 2016

Member

JS will faithfully represent integers up to 53 bits even if the engine opts to use ints instead of doubles internally to store them at 32 bits and below. As a result, other core functions that call into built-in Math stuff will work on larger values whereas things that are done by hand like truncate will clamp to 32 bits. An SSCCE that demonstrates this situation:

truncate 900719925474097.4 == round 900719925474097.4 -- False
truncate 900719925474097.4 == floor 900719925474097.4 -- Also False

round and floorare both implemented directly as the functions in Math so they preserve the significand.

Member

lukewestby commented Sep 22, 2016

JS will faithfully represent integers up to 53 bits even if the engine opts to use ints instead of doubles internally to store them at 32 bits and below. As a result, other core functions that call into built-in Math stuff will work on larger values whereas things that are done by hand like truncate will clamp to 32 bits. An SSCCE that demonstrates this situation:

truncate 900719925474097.4 == round 900719925474097.4 -- False
truncate 900719925474097.4 == floor 900719925474097.4 -- Also False

round and floorare both implemented directly as the functions in Math so they preserve the significand.

@elm elm locked and limited conversation to collaborators Jan 5, 2018

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