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

Fix #1671: Fix {Float,Double}.toInt for large abs values. #1676

Merged
merged 2 commits into from May 22, 2015

Conversation

sjrd
Copy link
Member

@sjrd sjrd commented May 20, 2015

The behavior of ToInt32(x) in ES does not match the behavior of (int) x on the JVM for values that are lower than Int.MinValue or larger than Int.MaxValue. On the JVM, and hence for .toInt, they are clamped at Int.MinValue or Int.MaxValue, respectively. So we have to handle these cases separately.

@sjrd
Copy link
Member Author

sjrd commented May 20, 2015

Review by @nicolasstucki

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1715/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2223/
Test FAILed.

@nicolasstucki
Copy link
Contributor

Isn't there be the same problems with Short and Byte? If so, we could fix it the same way.

@sjrd
Copy link
Member Author

sjrd commented May 21, 2015

Short and Byte (and Char) don't have that problem, because x.toShort is basically defined as x.toInt.toShort (see JLS section 5.1.3). And that's how we implement them: x.toShort is compiled as DoubleToInt(x) << 16 >> 16, for example.

@nicolasstucki
Copy link
Contributor

Oh, nice and simple.
LGTM

@sjrd
Copy link
Member Author

sjrd commented May 21, 2015

Updated. I had to add a commit fixing some of our low-level algorithms, which were relying on the buggy behavior of x.toInt.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1718/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2227/
Test FAILed.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1720/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2229/
Test FAILed.

Some low-level algorithms in java.util.Random and
scala.scalajs.runtime.Bits relied on the buggy behavior of
(x: Double).toInt. This commit fixes this by explicitly using a
`rawToInt` function that uses `x | 0` explicitly with js.Dynamic.
@sjrd
Copy link
Member Author

sjrd commented May 22, 2015

Rebased.
@nicolasstucki Can you review the first commit, please (Do not rely ...). The second commit hasn't changed since your first review.

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1724/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2234/
Test FAILed.

The behavior of ToInt32(x) in ES does not match the behavior of
(int) x on the JVM for values that are lower than Int.MinValue
or larger than Int.MaxValue. On the JVM, and hence for .toInt,
they are clamped at Int.MinValue or Int.MaxValue, respectively.
So we have to handle these cases separately.
@nicolasstucki
Copy link
Contributor

LGTM

@scala-jenkins
Copy link

Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1725/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2235/
Test PASSed.

gzm0 added a commit that referenced this pull request May 22, 2015
Fix #1671: Fix {Float,Double}.toInt for large abs values.
@gzm0 gzm0 merged commit 51f0917 into scala-js:master May 22, 2015
@sjrd sjrd deleted the double-to-int branch May 28, 2015 12:15
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

4 participants