BigInt to Number conversion

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
2 months ago

People

(Reporter: terpri, Assigned: terpri)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

11 months ago
Implement BigInt to Number conversion for the Number constructor.
(Assignee)

Comment 1

11 months ago
ToNumeric is a generalized version of ToNumber that converts its
argument to a numeric primitive value (either a Number or a BigInt).
Attachment #8983488 - Flags: review?(jdemooij)
(Assignee)

Comment 2

11 months ago
Attachment #8983489 - Flags: review?(jdemooij)
(Assignee)

Updated

11 months ago
Blocks: js-bigint
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 {}
Attachment #8983488 - Flags: review?(jdemooij) → review+
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?).
Attachment #8983489 - Flags: review?(jdemooij) → review+

Updated

11 months ago
Assignee: nobody → robin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 5

10 months ago
remove a pair of braces, carry forward r+
(Assignee)

Updated

10 months ago
Attachment #8983488 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8984928 - Flags: review+
(Assignee)

Comment 6

10 months ago
(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.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
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?
Flags: needinfo?(robin)
(Assignee)

Comment 8

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

Updated

10 months ago
Attachment #8983489 - Attachment is obsolete: true
Flags: needinfo?(robin)
(Assignee)

Updated

10 months ago
Attachment #8984928 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8985026 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8985027 - Flags: review+
(Assignee)

Comment 10

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

Updated

10 months ago
Attachment #8985027 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8985026 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8985041 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8985040 - Flags: review+
(Assignee)

Comment 12

10 months ago
(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.
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 13

10 months ago
Pushed by rgurzau@mozilla.com:
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
Keywords: checkin-needed

Comment 14

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/631e90e9e074
https://hg.mozilla.org/mozilla-central/rev/7cdfba2700eb
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Comment 15

2 months ago

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

2 months ago

(In reply to 4esn0k from comment #15)

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

You need to log in before you can comment on or make changes to this bug.