-
Notifications
You must be signed in to change notification settings - Fork 850
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
Math.imul sometimes returns incorrectly wrapped negative numbers #448
Comments
This is the last version that does not introduce `Math.imul`. The implementation added in Rhino 1.7.7, and still used in 1.7.9, is faulty. See mozilla/rhino#448.
In addition, calling $ node
> Math.imul(4)
0
> Math.imul()
0
> var o = { valueOf() { throw new RangeError("meh"); } };
undefined
> Math.imul(o, 5)
RangeError: meh
at Object.valueOf (repl:1:29)
at Math.imul (<anonymous>)
> Math.imul(o)
RangeError: meh
at Object.valueOf (repl:1:29)
at Math.imul (<anonymous>) FWIW, I believe the following implementation of private Object js_imul(Object[] args)
{
if ((args == null) || (args.length == 0)) {
return ScriptRuntime.toNumber(0);
}
int x = Conversions.toInt32(args[0]);
int y = args.length < 2 ? 0 : Conversions.toInt32(args[1]);
return ScriptRuntime.toNumber(x * y);
} |
As an added bonus, return an `int` from `js_imul` to avoid boxing. At call site, it is assigned to a `double`, not an `Object`, so eagerly boxing the result inside `js_imul` is counter-productive.
Expected, with Node.js v10.0.0:
Actual, with Rhino 1.7.9 (also 1.7.7+, since the introduction of
Math.imul
in 79e11ac):Negative numbers smaller than -2**31 should be wrapped around to the positive side, basically using
ToInt32
.The text was updated successfully, but these errors were encountered: