Implement BigInt64 and BigUint64 DataView methods. #2235
Conversation
| return realThis.js_getBigInt(false, args); | ||
| } | ||
|
|
||
| private Object js_getBigInt(boolean signed, Object[] args) { |
There was a problem hiding this comment.
i think this shoud not be namend js_...; methods starting with js_ are usually mappend to js lambdas, same for the setter
There was a problem hiding this comment.
I agree, but it's consistent with the naming of everything else in this file.
| // Interpret as unsigned 64-bit integer | ||
| if ((base & 0x8000000000000000L) == 0) { | ||
| return java.math.BigInteger.valueOf(base); | ||
| } else { |
There was a problem hiding this comment.
minor/personal taste: no need for else here
| return Undefined.instance; | ||
| } | ||
|
|
||
| private void js_setBigInt(boolean signed, Object[] args) { |
There was a problem hiding this comment.
why is the 'signed' parameter not used at all (maybe i'm blind?)
There was a problem hiding this comment.
I'm not sure. I think I might rewrite that function.
There was a problem hiding this comment.
Okay, so longValue did what was needed providing it never saw a signed big int with 64 or more bits, but that seems fragile and slightly obscure so I've made the sign handling explicit.
|
|
||
| long base = val.longValue(); | ||
| // base will be the 64 LSB of the unsigned big integer. | ||
| long base = val.abs().longValue(); |
There was a problem hiding this comment.
My AI code review tool (Gemini CLI this time) doesn't like this last change -- and indeed, it doesn't wrap properly, and the output doesn't match Node.js (Not that Node is always right but they do have a lot of people to do QA.) Output does match if I take this bit out and go back to "base=val.longValue()".
I'll see if I can attach the test case it generated -- I rather like it.
Do you mind if I make a version of this PR with those tests in it? Then I really want to get to the release!
There was a problem hiding this comment.
FWIW:
The original code was simple and correct:
long base = val.longValue();
This correctly truncates any BigInteger to its lower 64 bits, which is
the exact behavior required by the ECMAScript specification for both
signed (setBigInt64) and unsigned (setBigUint64) operations. It properly
handles two's complement for signed values and performs the modulo 2^64
operation for unsigned values.
The new code is complex and flawed:
1 long base = val.abs().longValue();
2 if (signed) {
3 base = (base & 0x7fffffffffffffffL) * val.signum();
4 }
This logic fails in several key cases:
- For
setBigInt64(signed): It fails to correctly write negative
numbers, most notably Long.MIN_VALUE. The combination of abs(),
masking with 0x7fffffffffffffffL, and multiplying by the signum
results in incorrect values. For example, trying to write
Long.MIN_VALUE results in 0 being written to the buffer. - For
setBigUint64(unsigned): It fails when the JavaScript BigInt is
negative. The spec requires taking the value modulo 2^64. For
instance, -1n should wrap around to the largest unsigned 64-bit
integer (0xFF...FF), which is represented by -1L in Java. The new
code takes the absolute value, causing 1L to be written instead.*
**
| private void js_setBigInt(boolean signed, Object[] args) { | ||
| int pos = ScriptRuntime.toIndex(isArg(args, 0) ? args[0] : Undefined.instance); | ||
|
|
||
| BigInteger val = ScriptRuntime.toBigInt(isArg(args, 1) ? args[1] : Undefined.instance); |
There was a problem hiding this comment.
Our implementation of this doesn't match the spec -- it should fail if the argument is a Number that's not a BigInt.
ScriptRuntime.toBigInt should impement the abstract ECMAScript operation "ToBigInt", in section 7.1.13 that fails on a Number.
The BigInt constructor should instead call "NumberToBigInt" in section 21.2, which is what we are doing in the constructor.
|
Given that this is ES2026 stuff, I'd like to wait until we get all the details right of this feature and have more comprehensive tests. BTW I have had a lot of success by using the "mjsunit" test suite in V8 to test for things like this -- in this case, it caught some things that test262 did not. |
|
Okay, I've gone and checked the spec, it seems to jump through a bunch of hoops that make the conversions to |
|
Yep, I confirmed a few different ways that "longValue" matches the correct result in all cases, and compared output to Node.js as well. I'll attach a small test program. |
aardvark179
force-pushed
the
aardvark179-dataview-bigint
branch
from
dd1d5f1 to
a34703d
Compare
zhurvla
left a comment
zhurvla
left a comment
There was a problem hiding this comment.
Looks good.
A few else after return can be avoided for readability.
https://github.com/mozilla/rhino/pull/2235/files#diff-a25991e9dccfaf928a5a19d200dd5bc22e2f563b9eacd80576115bf952f5189cR448
|
I'm good with this now. Thanks for all the work! |
6 participants
This PR includes just the big int related changes from #2218.