1466893 - BigInt to Number conversion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Robin Templeton [:terpri]]

Implement BigInt to Number conversion for the Number constructor.

Comment 1

[Robin Templeton [:terpri]]

Attachment: Part 1: Implement ToNumeric operation.

ToNumeric is a generalized version of ToNumber that converts its
argument to a numeric primitive value (either a Number or a BigInt).

Comment 2

[Robin Templeton [:terpri]]

Attachment: Part 2: Convert BigInt arguments to the Number constructor.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8983488 [details] [diff] [review]
Part 1: Implement ToNumeric operation.

Review of attachment 8983488 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, I forgot to get to this yesterday.

::: js/src/jsnum.cpp
@@ +1626,5 @@
> +    }
> +
> +    // Step 2.
> +#ifdef ENABLE_BIGINT
> +    if (vp.isBigInt()) {

Nit: no {}

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8983489 [details] [diff] [review]
Part 2: Convert BigInt arguments to the Number constructor.

Review of attachment 8983489 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/BigIntType.cpp
@@ +197,5 @@
> +double
> +BigInt::numberValue(BigInt* x)
> +{
> +    // mpz_get_d may cause a hardware overflow trap, so use
> +    // mpz_get_d_2exp to get the fractional part and exponent

Is this (much) slower than mpz_get_d? Wondering if we should have a follow-up bug to optimize this later for BigInts that are in a "safe" range (if that even makes sense?).

Comment 5

[Robin Templeton [:terpri]]

Attachment: Part 1: Implement ToNumeric operation.

remove a pair of braces, carry forward r+

Comment 6

[Robin Templeton [:terpri]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Comment on attachment 8983489 [details] [diff] [review]
> Part 2: Convert BigInt arguments to the Number constructor.
> 
> Review of attachment 8983489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/BigIntType.cpp
> @@ +197,5 @@
> > +double
> > +BigInt::numberValue(BigInt* x)
> > +{
> > +    // mpz_get_d may cause a hardware overflow trap, so use
> > +    // mpz_get_d_2exp to get the fractional part and exponent
> 
> Is this (much) slower than mpz_get_d? Wondering if we should have a
> follow-up bug to optimize this later for BigInts that are in a "safe" range
> (if that even makes sense?).

I'm not sure which is better, but both functions are basically wrappers for the internal mpn_get_d function, so I'll probably revisit this if SpiderMonkey switches to using the low-level GMP API. It should make sense to optimize it for small integers if GMP doesn't already.

Comment 7

[Eliza Balazs [:ebalazs_]]

Hi, when trying to apply part 1, I get the following error:
applying bug-1466893---Part-2-Convert-BigInt-arguments-to-t.patch
patching file js/src/vm/BigIntType.h
Hunk #1 FAILED at 67
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/BigIntType.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1466893---Part-2-Convert-BigInt-arguments-to-t.patch
:terpri, can you have a look at this?

Comment 8

[Robin Templeton [:terpri]]

Attachment: Part 1: Implement ToNumeric operation. r=jandem

ToNumeric is a generalized version of ToNumber that converts its
argument to a numeric primitive value (either a Number or a BigInt).

Comment 9

[Robin Templeton [:terpri]]

Attachment: Part 2: Convert BigInt arguments to the Number constructor. r=jandem

Comment 10

[Robin Templeton [:terpri]]

Attachment: Part 1: Implement ToNumeric operation

ToNumeric is a generalized version of ToNumber that converts its
argument to a numeric primitive value (either a Number or a BigInt).

Comment 11

[Robin Templeton [:terpri]]

Attachment: Part 2: Convert BigInt arguments to the Number constructor

Comment 12

[Robin Templeton [:terpri]]

(In reply to Eliza Balazs [:ebalazs_] from comment #7)
> Hi, when trying to apply part 1, I get the following error:
> applying bug-1466893---Part-2-Convert-BigInt-arguments-to-t.patch
> patching file js/src/vm/BigIntType.h
> Hunk #1 FAILED at 67
> 1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/BigIntType.h.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh
> bug-1466893---Part-2-Convert-BigInt-arguments-to-t.patch
> :terpri, can you have a look at this?

I rebased this onto mozilla-central (and exported through hg instead of git); this should apply correctly now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/631e90e9e074
Part 1: Implement ToNumeric operation. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdfba2700eb
Part 2: Convert BigInt arguments to the Number constructor. r=jandem

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/631e90e9e074
https://hg.mozilla.org/mozilla-central/rev/7cdfba2700eb

Comment 15

[4esn0k]

small bug:

Number(100000000000000008193n) returns 100000000000000000000, while it should return 100000000000000016384

Note: Number(100000000000000008192n) === 100000000000000000000

see https://tc39.github.io/proposal-bigint/#sec-number-constructor-number-value
(rounding should be to nearest, ties to even)

Well, this bug is not important at all, :-)
Thank you!

Comment 16

[4esn0k]

(In reply to 4esn0k from comment #15)

Oh, you have switched to V8's implementation, so ignore my previous comment.

Comment 17

[4esn0k]

Firefox beta has the next issue:
Number(0b1000000000000000000000000000000000000000000000000000011n) gives 0b1000000000000000000000000000000000000000000000000000000, not
0b1000000000000000000000000000000000000000000000000000100

Comment 18

[Jan de Mooij [:jandem]]

wingo, robin: thoughts on comment 17?

Comment 19

[Jeff Walden [:Waldo]]

Filed bug 1558538 for comment 17, I done goofed bigly.