BigInt to Number conversion

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
7 days ago

People

(Reporter: terpri, Assigned: terpri)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Assignee

Description

Last year
Implement BigInt to Number conversion for the Number constructor.
Assignee

Comment 1

Last year
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

Updated

Last year
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+
Assignee: nobody → robin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 5

Last year
remove a pair of braces, carry forward r+
Assignee

Updated

Last year
Attachment #8983488 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8984928 - Flags: review+
Assignee

Comment 6

Last year
(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

Last year
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

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

Updated

Last year
Attachment #8983489 - Attachment is obsolete: true
Flags: needinfo?(robin)
Assignee

Updated

Last year
Attachment #8984928 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8985026 - Flags: review+
Assignee

Updated

Last year
Attachment #8985027 - Flags: review+
Assignee

Comment 10

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

Updated

Last year
Attachment #8985027 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8985026 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8985041 - Flags: review+
Assignee

Updated

Last year
Attachment #8985040 - Flags: review+
Assignee

Comment 12

Last year
(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

Last year
Keywords: checkin-needed

Comment 13

Last year
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

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

Comment 15

4 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

4 months ago

(In reply to 4esn0k from comment #15)

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

Comment 17

10 days ago

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

wingo, robin: thoughts on comment 17?

Flags: needinfo?(wingo)
Flags: needinfo?(robin)

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

Flags: needinfo?(wingo)
Flags: needinfo?(robin)
You need to log in before you can comment on or make changes to this bug.