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 #373: Implement BigInteger and BigDecimal. #1549

Merged
merged 2 commits into from May 8, 2015
Merged

Conversation

@ghost
Copy link

@ghost ghost commented Mar 16, 2015

Implements #373

Notes:

  • There are four tests marked as xit, all relating to float infinity being casted to doubl infinity
  • There are a couple of comments of "Todo:" where some functionality could go into main javalib.
@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented Mar 16, 2015

Can one of the admins verify this patch?

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

Well, that comes at a rather bad time for me. I probably won't be able to review this until March 25, date of an academic deadline.

(next comment is intended for the build bot)

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

test this please

@ghost
Copy link
Author

@ghost ghost commented Mar 16, 2015

@sjrd np at all - I'm just glad to have got my bit done already!

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Mar 16, 2015

I might have some resources to review. @sjrd how about you hold off and I start reviewing and we'll see where I get.

def signum(i: scala.Long): scala.Long =
if (i < 0L) -1L else if (i == 0L) 0L else 1L
def signum(i: scala.Long): Int =
if (i < 0L) -1 else if (i == 0L) 0 else 1

This comment has been minimized.

@gzm0

gzm0 Mar 16, 2015
Contributor

This is a bug :) (a rather serious one, as it will probably lead to type errors at runtime). Could you file this and PR separately. @sjrd I assume we don't have a chance that this makes it into 0.6.2.

This comment has been minimized.

@sjrd

sjrd Mar 16, 2015
Member

Er, no, it won't go into 0.6.2 anymore, sorry.

This comment has been minimized.

@ghost

ghost Mar 16, 2015
Author

OK - I'll PR this one on its own

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

@gzm0 That would be good, thanks :-D

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

Some feedback from the CI:

[warn] /localhome/jenkins/b/workspace/scalajs-task-worker/test-suite/src/test/scala/org/scalajs/testsuite/javalib/math/BigDecimalCompareTest.scala:166: comparing values of types java.math.BigDecimal and String using `==' will always yield false
[warn]       assertFalse(aNumber == b)
[warn]                           ^

Also, BigDecimalArithmeticTest.testDivideZero fails:

[error]  x testDivideZero
[error]     Expected { toStringImage$2 : '0', $$undhashCode$2 : 0, intVal$2 : { digits$2 : { u : [ 0 ] }, numberLength$2 : 1, sign$2 : 0, firstNonzeroDigit$2 : -2, java$math$BigInteger$$$undhashCode$2 : 0 }, java$math$BigDecimal$$$undbitLength$2 : 0, java$math$BigDecimal$$$undsmallValue$2 : { l$2 : 0, m$2 : 0, h$2 : 0 }, java$math$BigDecimal$$$undscale$2 : 0, $$undprecision$2 : 0 } to equal { toStringImage$2 : null, $$undhashCode$2 : 0, intVal$2 : { digits$2 : { u : [ 0 ] }, numberLength$2 : 1, sign$2 : 0, firstNonzeroDigit$2 : -2, java$math$BigInteger$$$undhashCode$2 : 0 }, java$math$BigDecimal$$$undbitLength$2 : 0, java$math$BigDecimal$$$undsmallValue$2 : { l$2 : 0, m$2 : 0, h$2 : 0 }, java$math$BigDecimal$$$undscale$2 : 0, $$undprecision$2 : 0 }.
@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

The bootstrap test fails with a linking error (locally, use ´set scalaJSStage in Global := FastOptStageandtoolsJS/test` to reproduce):

[info] Fast optimizing /localhome/jenkins/a/workspace/scalajs-task-worker/test-suite/target/scala-2.11/scalajs-test-suite-test-fastopt.js
[info] Fast optimizing /localhome/jenkins/a/workspace/scalajs-task-worker/tools/js/target/scala-2.11/scalajs-tools-test-fastopt.js
[error] Referring to non-existent method jl_Long$.toString__J__I__T
[error]   called from Ljava_math_Conversion$.bigInteger2String__Ljava_math_BigInteger__I__T
[error]   called from Ljava_math_BigInteger.toString__I__T
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply$mcV$sp__V
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply__V
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply__O
[error]   called from Lorg_scalajs_testinterface_internal_Slave.jsonArg$1__p2__F0__sjs_js_Dynamic
[error]   called from Lorg_scalajs_testinterface_internal_Slave.$$anonfun$1__p2__T__F0__V
[error]   called from Lorg_scalajs_testinterface_internal_Slave.handleMsgImpl__T__F0__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.handleMsg__p1__T__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.$$anonfun$1__p1__T__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.init__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.$$js$exported$meth$init__O
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.init
[error]   exported to JavaScript with @JSExport
[error] involving instantiated classes:
[error]   Ljava_math_Conversion$
[error]   Ljava_math_BigInteger
[error]     (already seen, not repeating call stack)
[error]   Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1
[error]   Lorg_scalajs_testinterface_internal_Slave
[error]   Lorg_scalajs_testinterface_internal_Master
[error] Referring to non-existent method jl_Long$.toString__J__I__T
[error]   called from Ljava_math_Conversion$.bigInteger2String__Ljava_math_BigInteger__I__T
[error]   called from Ljava_math_BigInteger.toString__I__T
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply$mcV$sp__V
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply__V
[error]   called from Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1.apply__O
[error]   called from Lorg_scalajs_testinterface_internal_Slave.jsonArg$1__p2__F0__sjs_js_Dynamic
[error]   called from Lorg_scalajs_testinterface_internal_Slave.$$anonfun$1__p2__T__F0__V
[error]   called from Lorg_scalajs_testinterface_internal_Slave.handleMsgImpl__T__F0__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.handleMsg__p1__T__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.$$anonfun$1__p1__T__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.init__V
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.$$js$exported$meth$init__O
[error]   called from Lorg_scalajs_testinterface_internal_BridgeBase.init
[error]   exported to JavaScript with @JSExport
[error] involving instantiated classes:
[error]   Ljava_math_Conversion$
[error]   Ljava_math_BigInteger
[error]     (already seen, not repeating call stack)
[error]   Lorg_scalajs_testsuite_javalib_math_BigIntegerToStringTest$$anonfun$1
[error]   Lorg_scalajs_testinterface_internal_Slave
[error]   Lorg_scalajs_testinterface_internal_Master
java.lang.RuntimeException: There were linking errors
@ghost
Copy link
Author

@ghost ghost commented Mar 16, 2015

[warn] assertFalse(aNumber == b)

Yes, it's supposed to always fail - this is one of the original tests. Shall I leave as is or remove it?

BigDecimalArithmeticTest.testDivideZero

This passes locally - any ideas on the environment that could cause this difference?

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 16, 2015

Yes, it's supposed to always fail - this is one of the original tests. Shall I leave as is or remove it?

Maybe call .equals directly to silence the warning, and add a comment saying why you do that.

This passes locally - any ideas on the environment that could cause this difference?

Scala version, maybe. It fails with 2.10.2, for example:
https://scala-webapps.epfl.ch/jenkins/job/scalajs-task-worker/32364/consoleFull

@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented Mar 16, 2015

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

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

@ghost
Copy link
Author

@ghost ghost commented Mar 16, 2015

The issue with the bootstap is a missing method in java.lang.Long.toString(long i, int radix)

@ghost
Copy link
Author

@ghost ghost commented Mar 17, 2015

@sjrd @gzm0

I've fixed all the above but I have implemented the following as they are missing from javalib - perhaps I should pull these out first and PR them as for Long.signum?:

  • Integer toString(i: scala.Int, radix: scala.Int): String . this just calls private[this] def toStringBase(i: scala.Int, base: scala.Int): String . Perhaps it is best just to rename this and make public?
  • Ingteger highestOneBit(i: Int): Int
  • Char forDigit(digit: Int): Char and def forDigit(digit: Int, radix: Int): Char
  • Long toString(value: Long, intRadix: Int): String

Move awkward is

 String  regionMatches(s: String, ignoreCase: Boolean,
                            toffset: Int,
                            other: String,
                            ooffset: Int,
                            len: Int): Boolean 
def regionMatches(s: String, toffset: Int,
                            other: String,
                            ooffset: Int,
                            len: Int): Boolean 

as i believe you treat Strings differently. Any thoughts?

In the meantime, I'll finish #1551

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 17, 2015

It would be good indeed to factor out the other small methods in a dedicated PR.

toString with radix cannot use toStringBase, because they handle negative numbers differently.

Methods of java.lang.String are implemented in RuntimeString. The compiler magically rewires calls like s.someMethod(args...) to RuntimeString.someMethod(s, args...).

@sjrd
Copy link
Member

@sjrd sjrd commented Mar 18, 2015

@inthenow You should not merge master into the branch used for this PR. You should rebase instead.

@ghost
Copy link
Author

@ghost ghost commented Mar 18, 2015

You should not merge master into the branch used for this PR. You should rebase instead.

Ahhhh... school boy error. And I had just finished everything!!!

@ghost
Copy link
Author

@ghost ghost commented Mar 18, 2015

Not sure if I have got git correct, but otherwise everything is in :)

@ghost
Copy link
Author

@ghost ghost commented Mar 18, 2015

Ok- git looks good and build passes all tests on all scala versions (sbt +testSuite/test)

@sjrd @gzm0 With you now - no rush as I know you have a 25.03 deadline

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented Mar 19, 2015

I'm sorry I have to say that, as I know the pain that this will inflict upon you to correct, but the commits are in the wrong order. The commits that add the auxiliary methods to the Javalib should come first in the history. Also, please reference the ticket numbers from the commits:

"Fix #xxx: msg" if the commit fixes a full issue, a comment in the body that mentions partial fixes. Further, please start commit messages upper-case and formulate in present tense. (Have a look at the history for examples).

I will start reviewing anyway, as most of it is orthogonal to how the commits are ordered.

@@ -213,6 +213,17 @@ object Character {
else -1
}

// ported from https://github.com/gwtproject/gwt/blob/master/user/super/com/google/gwt/emul/java/lang/Character.java
def forDigit(digit: Int, radix: Int): Char = {
if ( radix < MIN_RADIX || radix > MAX_RADIX || digit < 0 || digit >= radix) {

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

No space before radix

if ( radix < MIN_RADIX || radix > MAX_RADIX || digit < 0 || digit >= radix) {
0
}
else {

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

on the same line than closing brace of then-part

}
else {
val overBaseTen = digit - 10
(if (overBaseTen < 0) '0' + digit else 'a' + overBaseTen).toChar

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

Have you considered using JavaScript's toString? As far as I can see, it does exactly what we want. To enable it, import js.JSNumberOps.enableJSNumberOps.

This comment has been minimized.

@ghost

ghost Mar 20, 2015
Author

It would be be possible, yes - but I wonder on the benefit/performance? The check to return 0 has to stay as JS throws rather than return 0. So in effect, this is only one line of code. My hunch would be to keep it as is - but your call.

This comment has been minimized.

@gzm0

gzm0 Mar 20, 2015
Contributor

You are right. Performance of your code is better. Lets leave it.
However, could you assign the result of the inner if to another val? parenthesized ifs are hard to read (and we don't have any of them AFAIK).

String.valueOf(buf, cursor, bufLen - cursor)
}
}

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

Have you considered using JavaScript's toString? As far as I can see, it does exactly what we want. To enable it, import js.JSNumberOps.enableJSNumberOps.

This comment has been minimized.

@ghost

ghost Mar 20, 2015
Author

Same argument could be applied here as for forDigit - but less so. I think I'll keep the check and change the body to JS

This comment has been minimized.

@gzm0

gzm0 Mar 20, 2015
Contributor

Yes. Please. This one will be faster than your custom implementation. (and much less code).

'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',
'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't',
'u', 'v', 'w', 'x', 'y', 'z')

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

This won't probably be needed anymore. Otherwise, consider writing this as: ('0' to '9' ++ 'a' to 'z').toArray

} else if (i == 0) {
0
} else {
var rtn: Int = 0x40000000

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

Consider writing var rtn = (1 << 31). It is clearer and will be constant folded anyway (how many zeros were there at the end ;-))

}
buf(pos) = forDigit(-_value.toInt)
pos -= 1
buf(pos) = '-'

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

This duplicated code could be avoided by only handling the negative case (don't handle the positive one, as you can't negate MIN_LONG).

This comment has been minimized.

@ghost

ghost Mar 20, 2015
Author

I'm not so sure on this one. Looking at the non-radix version, it makes sense how that was done. Here it is not so clean as both the radix and the value are negated, so again I'm not so convinced on the benefit. I would be tempted to keep as is (mainly as it is a very low level class) - your call.

This comment has been minimized.

@gzm0

gzm0 Mar 20, 2015
Contributor

I just don't see why we want the essentially same code twice... if you are concerned about the inefficiency of the negation: the constant folder will take care of this.

def forDigit(digit: Int): Char = {
val overBaseTen = digit - 10
(if (overBaseTen < 0) '0' + digit else 'a' + overBaseTen).toChar
}

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

Use Character.forDigit instead.

This comment has been minimized.

@ghost

ghost Mar 20, 2015
Author

Unfortunately, not possible. The java implementation specifically split forDigit as the one here has no range checks, Character.forDigit does. If its OK, I keep as is - but add the missing comment as to why its needed

This comment has been minimized.

@gzm0

gzm0 Mar 20, 2015
Contributor

Oh. ok. makes sense. However, please indent by 2 only in the inline def.

@@ -166,6 +166,32 @@ private[runtime] object RuntimeString {
Pattern.matches(regex, thiz)
}

// Both regionMatches ported from https://github.com/gwtproject/gwt/blob/master/user/super/com/google/gwt/emul/java/lang/String.java
def regionMatches(thiz: String, ignoreCase: Boolean,
toffset: Int, other: String,

This comment has been minimized.

@gzm0

gzm0 Mar 19, 2015
Contributor

Indent by 4 spaces only (so you can probably make 2 lines out of the 3).

@gzm0
Copy link
Contributor

@gzm0 gzm0 commented May 5, 2015

Just a comment on the commit messages: If you end up including ScalaNumericConversions.scala, please make sure to include the commit SHA1 of the scala/scala repo you took the file from. This will make it much easier to detect and backport changes if necessary.

@ghost
Copy link
Author

@ghost ghost commented May 5, 2015

fyi - I added it as I saw ScalaNumber.scala was also just a copy. Anyway, I'll leave this in your very capable hands - will be back online Wed p.m.

@sjrd
Copy link
Member

@sjrd sjrd commented May 5, 2015

I added it as I saw ScalaNumber.scala was also just a copy.

It's not a copy, it's a rewrite in Scala of a file that is originally in Java ;-) See https://github.com/scala/scala/blob/2.11.x/src/library/scala/math/ScalaNumber.java

@ghost
Copy link
Author

@ghost ghost commented May 5, 2015

Just looking through some code, I wonder if the issue is related to scala BigDecimal:

final class BigDecimal(val bigDecimal: BigDec, val mc: MathContext)
extends ScalaNumber with ScalaNumericConversions with Serializable {
@sjrd
Copy link
Member

@sjrd sjrd commented May 5, 2015

Locally, all the tests pass here when I remove the overrides file. Both on 2.11.6 and 2.10.5. Maybe you did this as an attempt of fixing something that was actually fixed by doing something else?

I suggest that you remove that commit (git rebase -i master and simply remove the line that picks that commit) and re-push.

@ghost
Copy link
Author

@ghost ghost commented May 5, 2015

OK - i did actually test yesterday that it was required by removing it and it was still needed - but all the better if it can go !

@ghost
Copy link
Author

@ghost ghost commented May 5, 2015

done.

@sjrd
Copy link
Member

@sjrd sjrd commented May 5, 2015

The bootstrapping tests do not pass on this PR. Example at https://scala-webapps.epfl.ch/jenkins/job/scalajs-task-worker/37782/console
To replicate locally:

> set scalaJSStage in Global := FastOptStage
> toolsJS/test

I verified locally: it does succeed on master, but fails on this PR's branch.

@sjrd
Copy link
Member

@sjrd sjrd commented May 5, 2015

@inthenow I will try and debug that one. It is probably caused by code under my responsibility, and only revealed by your additions.

@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented May 5, 2015

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

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

@ghost
Copy link
Author

@ghost ghost commented May 6, 2015

@sjrd Any development on this? Or anything I can look at?

@sjrd
Copy link
Member

@sjrd sjrd commented May 6, 2015

I didn't have time today, unfortunately. I will investigate this tomorrow.

@sjrd
Copy link
Member

@sjrd sjrd commented May 7, 2015

OK you can grab my branch at https://github.com/sjrd/scala-js/commits/BigNumerics, and push it here for the PR. It contains an additional commit that fixes how test detection for toolsJS/test works, so that it works with your commit. I haven't touched your commit, just rebased it on top of mine.

@ghost
Copy link
Author

@ghost ghost commented May 7, 2015

done

@sjrd
Copy link
Member

@sjrd sjrd commented May 7, 2015

The history's all weird: there are duplicates of three commits already in master. To fix it, do:

$ git fetch scalajs
$ git rebase -i scalajs/master

and remove all lines except the two commits to keep: the one with TestDetector and the one with big numbers.

@ghost
Copy link
Author

@ghost ghost commented May 7, 2015

done - before had just pushed what was on you branch

@sjrd
Copy link
Member

@sjrd sjrd commented May 7, 2015

retest this please

@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented May 7, 2015

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

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

@sjrd
Copy link
Member

@sjrd sjrd commented May 7, 2015

@@ -193,5 +193,28 @@ object IntegerTest extends JasmineTest {
expect(Integer.toString(-2147483648, 2)).toEqual("-10000000000000000000000000000000")
expect(Integer.toString(-2147483648, 10)).toEqual("-2147483648")
}

it("should provide `toString`") {

This comment has been minimized.

@ghost

ghost May 8, 2015
Author

This test is redundant as this test was split into the two above - with/without radix. Will remove it.

sjrd and others added 2 commits May 7, 2015
Previously, it was only able to detect tests precisely in the
org.scalajs.testsuite package. This commit makes it able to look
for nested packages, and for example detect tests in
org.scalajs.testsuite.math.
@ghost
Copy link
Author

@ghost ghost commented May 8, 2015

done

@sjrd
Copy link
Member

@sjrd sjrd commented May 8, 2015

LGTM

@sjrd
Copy link
Member

@sjrd sjrd commented May 8, 2015

retest this please

@scala-jenkins
Copy link

@scala-jenkins scala-jenkins commented May 8, 2015

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

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

@sjrd sjrd changed the title BigDecimal and BigInteger Fix #373: scala's BigInt and BigDecimal are not implemented in Scala.js May 8, 2015
@sjrd sjrd changed the title Fix #373: scala's BigInt and BigDecimal are not implemented in Scala.js Fix #373: Implement BigInteger and BigDecimal. May 8, 2015
sjrd added a commit that referenced this pull request May 8, 2015
Fix #373: Implement BigInteger and BigDecimal.
@sjrd sjrd merged commit 3c9493e into scala-js:master May 8, 2015
1 check passed
1 check passed
default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.