-
Notifications
You must be signed in to change notification settings - Fork 385
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
Java random #780
Java random #780
Conversation
Review by @sjrd |
// see nextGaussian() | ||
private var nextNextGaussian: Double = _ | ||
private var haveNextNextGaussian: Boolean = false | ||
|
||
def this(seed: Long) = this() // ignore seed | ||
def this(seed_in: Long) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this one the primary constructor, then. Otherwise the initialization of seed
unnecessarily creates a random seed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the recommendation of member ordering there?
- vals
- primary ctor code
- ctors
- methods
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is usually the order I use, indeed.
Test FAILed. |
(randomInt().toLong << 32) | (randomInt().toLong & 0xffffffffL) | ||
|
||
private def randomInt(): Int = | ||
(Math.floor(Math.random() * 4294967296.0) - 2147483648.0).toInt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh oh! Must be js.Math.random()
! Otherwise this thing enters an infinite recursion, because Math.random()
uses a java.util.Random
created without seed, so with a random seed, which is created with randomInt()
, which calls Math.random()
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't java.lang.Math
shadowed by the import? (Importing scala.scala.js.Math
is your madness :P)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Well, fix my madness, then.
That's all. |
checksizes must be adapted. |
Test FAILed. |
This is bad: Reversi doesn't use |
It might not use it directly. That does not mean some corner of the collection library isn't using random numbers for some non-obvious reason. If you activate the |
Or simpler, remove the class altogether and look at the warnings emitted by fastOpt. |
If I rename whole |
It's the initializer of the --- master-fastopt.js 2014-06-29 22:40:31.086560212 +0200
+++ random-fastopt.js 2014-06-29 22:38:09.082565288 +0200
@@ -4221,8 +4221,10 @@
/** @constructor */
ScalaJS.c.jl_Math$ = (function() {
ScalaJS.c.O.call(this);
+ this.internalRandom$1 = null;
this.E$1 = 0.0;
- this.PI$1 = 0.0
+ this.PI$1 = 0.0;
+ this.bitmap$0$1 = false
});
ScalaJS.c.jl_Math$.prototype = new ScalaJS.h.O();
ScalaJS.c.jl_Math$.prototype.constructor = ScalaJS.c.jl_Math$; |
Updated |
Before merging, please verify that the change in size for |
Test PASSed. |
Checksizes adaptation is justified for |
LGTM :) Funny one, the change in size. ^^ |
Yep... Cost me a good hour at least... :P |
Test PASSed. |
No description provided.