Bug 175359

Summary:

[ESNext][BigInt] Implement BigIntConstructor and BigIntPrototype

Product:

WebKit

Reporter:

Caio Lima <ticaiolima>

Component:

JavaScriptCore

Assignee:

Caio Lima <ticaiolima>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, ews-watchlist, fpizlo, guijemont, jfbastien, joepeck, keith_miller, mark.lam, msaboff, rmorisset, rniwa, saam, ticaiolima, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

179000

Bug Blocks:

179001, 179004, 180731

Attachments:

Description	Flags
WIP - It starts	none
WIP - Added parseInt	none
RFC - Patch proposal	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2	none
Patch	none
Patch	none
Patch	none
Patch	ysuzuki: review+
Patch for landing	none
Patch for landing	none
Patch for landing	none

Description Caio Lima 2017-08-08 17:05:40 PDT

The BigInt proposal advanced to stage 3 and we should consider implement it.

Reference:
https://github.com/tc39/proposal-bigint

Comment 1 Caio Lima 2017-10-22 16:44:10 PDT

The idea here is to implement the Front-End parsing and also the JSCell BigInt implementation based on V8 implementation[1].

Spec operations and remaining arithmetic algorithms should be implemented in a separated Patch.

Comment 2 Caio Lima 2017-10-22 16:51:19 PDT

Created attachment 324535 [details]
WIP - It starts

I'm proposing the implementation of BigInt as new JSCell specialization. The BigInt content is allocated contiguously on memory. I'm also implementing some basic operations on JSBigInt such as setDigit and getDigit and ToString generic. My ideia here is to implement the BigInt literal parsing and generation on constant pool with toString support. I will also implement StringToBigInt parser as well.

Comment 3 Caio Lima 2017-10-24 20:11:33 PDT

Created attachment 324783 [details]
WIP - Added parseInt

I'm adding parseInt function now. It will be required because I'm going to support BigInt literals in this Patch using Constant Pool.
Also, I updated the copyright properly.

Comment 4 Caio Lima 2017-10-27 14:01:52 PDT

Created attachment 325197 [details]
RFC - Patch proposal

Here, I started the implementation of BigIntPrototype and BigIntConstructor. Now, it's possible to create an BigInt using ```BigInt(<number>)``` on jsc. It's not spec compliant yet.

I'm planning to finish the BigIntConstructor in this patch and then create tests. Parsing BigInt literals will be handled in a new Bug.

I would like to get the first Batch of review now, if possible.

Comment 5 Build Bot 2017-10-27 14:04:31 PDT

Attachment 325197 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/BigIntConstructor.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/BigIntConstructor.cpp:104:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:74:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:135:  twodigit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:139:  twodigit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:154:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:167:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:170:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:170:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:172:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:222:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:241:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:251:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:252:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:270:  s_zero_mask is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:281:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:291:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:332:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:334:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:338:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:361:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:377:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:386:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:387:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:388:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:389:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:390:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:446:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:464:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:469:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:471:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:491:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:493:  More than one command on the same line in if  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:493:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:511:  More than one command on the same line in while  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:522:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:547:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:122:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:124:  digit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:140:  The parameter name "n" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 46 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 6 Caio Lima 2017-10-30 19:27:45 PDT

Created attachment 325411 [details]
Patch

Comment 7 Build Bot 2017-10-30 19:29:29 PDT

Attachment 325411 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:136:  twodigit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:140:  twodigit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:393:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:394:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:395:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:396:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:397:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:194:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:285:  digit_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 8 Saam Barati 2017-10-31 10:07:07 PDT

Comment on attachment 325411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325411&action=review

Will look more at this soon

> JSTests/stress/big-int-constructor.js:2
> +// This test requires --useBigInt=true to run properly.
> +//@ skip

You can add a bigInt run mode for now and just do `@ runBigInt`

Comment 9 Yusuke Suzuki 2017-11-01 06:33:11 PDT

Comment on attachment 325411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325411&action=review

> Source/JavaScriptCore/runtime/JSBigInt.cpp:136
> +typedef uint64_t twodigit_t;

TwoDigit is preferable.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:139
> +#elif defined(__SIZEOF_INT128__)
> +// Both Clang and GCC support this on x64.
> +#define HAVE_TWODIGIT_T 1

We have `HAVE(INT128_T)`. Using it instead if better.

> Source/JavaScriptCore/runtime/JSBigInt.h:285
> +    typedef uintptr_t digit_t;

Digit is preferable.

Comment 10 Yusuke Suzuki 2017-11-01 08:57:29 PDT

Comment on attachment 325411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325411&action=review

> Source/JavaScriptCore/runtime/JSBigInt.cpp:130
> +#if USE(JSVALUE32_64)
> +ALWAYS_INLINE static void countLeadingZerosPartial(uint32_t& value, uint8_t& zeros, const uint8_t N)
> +{
> +    if (value & ~((1 << N) - 1)) /* check for any of the top N bits (of 2N bits) are set */
> +        value >>= N;             /* if any were set, lose the bottom N */
> +    else                         /* if none of the top N bits are set, */
> +        zeros += N;              /* then we have identified N leading zeros */
> +}
> +
> +static uint8_t countLeadingZeros(uint32_t value)
> +{
> +    if (!value)
> +        return 32;
> +
> +    uint8_t zeros = 0;
> +    countLeadingZerosPartial(value, zeros, 16);
> +    countLeadingZerosPartial(value, zeros, 8);
> +    countLeadingZerosPartial(value, zeros, 4);
> +    countLeadingZerosPartial(value, zeros, 2);
> +    countLeadingZerosPartial(value, zeros, 1);
> +    return zeros;
> +}
> +#else
> +ALWAYS_INLINE static void countLeadingZerosPartial(uint64_t& value, uint8_t& zeros, const uint8_t N)
> +{
> +    if (value & ~((1 << N) - 1)) /* check for any of the top N bits (of 2N bits) are set */
> +        value >>= N;             /* if any were set, lose the bottom N */
> +    else                         /* if none of the top N bits are set, */
> +        zeros += N;              /* then we have identified N leading zeros */
> +}
> +
> +static uint8_t countLeadingZeros(uint64_t value)
> +{
> +    if (!value)
> +        return 64;
> +
> +    uint8_t zeros = 0;
> +    countLeadingZerosPartial(value, zeros, 32);
> +    countLeadingZerosPartial(value, zeros, 16);
> +    countLeadingZerosPartial(value, zeros, 8);
> +    countLeadingZerosPartial(value, zeros, 4);
> +    countLeadingZerosPartial(value, zeros, 2);
> +    countLeadingZerosPartial(value, zeros, 1);
> +    return zeros;
> +}
> +#endif

CountLeadingZero should be implemented with `__builtin_clz` / `__builtin_clzll`.
Let's implement clz64 in MathCommon.h and use it. It already has clz32.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:239
> +#if CPU(X86_64) && (__GNUC__ || __clang__)

Do not use `(__GNUC__ || __clang__)` directly.
Use `#if CPU(X86_64) && COMPILER(GCC_OR_CLANG)`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:250
> +#elif CPU(X86) && (__GNUC__ || __clang__)

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:401
> +constexpr uint8_t kMaxBitsPerChar[] = {
> +    0,   0,   32,  51,  64,  75,  83,  90,  96,  // 0..8
> +    102, 107, 111, 115, 119, 122, 126, 128,      // 9..16
> +    131, 134, 136, 139, 141, 143, 145, 147,      // 17..24
> +    149, 151, 153, 154, 156, 158, 159, 160,      // 25..32
> +    162, 163, 165, 166,                          // 33..36
> +};
> +
> +static const int kBitsPerCharTableShift = 5;
> +static const size_t kBitsPerCharTableMultiplier = 1u << kBitsPerCharTableShift;

Name should not be prefixed with `k`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:435
> +static const char kConversionChars[] = "0123456789abcdefghijklmnopqrstuvwxyz";

Ditto

> Source/JavaScriptCore/runtime/JSBigInt.h:291
> +    static const int kBitsPerByte = 8;
> +    static const int kDigitSize = sizeof(digit_t);
> +    static const int kDigitBits = kDigitSize * kBitsPerByte;
> +    static const int kHalfDigitBits = kDigitBits / 2;
> +    static const digit_t kHalfDigitMask = (1ull << kHalfDigitBits) - 1;
> +    static const int kMaxInt = 0x7FFFFFFF;

Use `BitsPerByte` style. Not `kXXX`.

> Source/JavaScriptCore/runtime/JSBigInt.h:299
> +    static const int kMaxLength = 1024 * 1024 / (sizeof(void*) * kBitsPerByte);

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.h:335
> +    CagedPtr<Gigacage::JSValue, void> m_data;

This should be `CagedBarrierPtr<Gigacage::Primitive, Digit>` since the data should be allocated from auxiliary space.

Comment 11 Caio Lima 2017-11-02 10:02:18 PDT

Comment on attachment 325411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325411&action=review

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:130
>> +#endif
> 
> CountLeadingZero should be implemented with `__builtin_clz` / `__builtin_clzll`.
> Let's implement clz64 in MathCommon.h and use it. It already has clz32.

Oh. that's good to know. Thanks!

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:239
>> +#if CPU(X86_64) && (__GNUC__ || __clang__)
> 
> Do not use `(__GNUC__ || __clang__)` directly.
> Use `#if CPU(X86_64) && COMPILER(GCC_OR_CLANG)`.

Changed.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:250
>> +#elif CPU(X86) && (__GNUC__ || __clang__)
> 
> Ditto.

Ok.

>> Source/JavaScriptCore/runtime/JSBigInt.h:335
>> +    CagedPtr<Gigacage::JSValue, void> m_data;
> 
> This should be `CagedBarrierPtr<Gigacage::Primitive, Digit>` since the data should be allocated from auxiliary space.

Done.

>> JSTests/stress/big-int-constructor.js:2
>> +//@ skip
> 
> You can add a bigInt run mode for now and just do `@ runBigInt`

Done.

Comment 12 Caio Lima 2017-11-02 10:04:18 PDT

Created attachment 325725 [details]
Patch

Comment 13 Build Bot 2017-11-02 10:06:22 PDT

Attachment 325725 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:350:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:351:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:352:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:353:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:194:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 14 Caio Lima 2017-11-07 14:38:01 PST

Ping review

Comment 15 Caio Lima 2017-11-09 07:53:13 PST

Created attachment 326447 [details]
Patch

Comment 16 Build Bot 2017-11-09 07:55:43 PST

Attachment 326447 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:350:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:351:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:352:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:353:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:194:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 17 Yusuke Suzuki 2017-11-09 08:10:32 PST

Comment on attachment 326447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326447&action=review

> Source/JavaScriptCore/runtime/JSBigInt.h:333
> +    CagedPtr<Gigacage::Primitive, Digit> m_data;

This should be `CagedBarrierPtr<Gigacage::Primitive, Digit>` since the data should be allocated from auxiliary space.

Comment 18 Caio Lima 2017-11-09 17:27:43 PST

Created attachment 326517 [details]
Patch

Comment 19 Caio Lima 2017-11-09 17:28:31 PST

(In reply to Yusuke Suzuki from comment #17)
> Comment on attachment 326447 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326447&action=review
> 
> > Source/JavaScriptCore/runtime/JSBigInt.h:333
> > +    CagedPtr<Gigacage::Primitive, Digit> m_data;
> 
> This should be `CagedBarrierPtr<Gigacage::Primitive, Digit>` since the data
> should be allocated from auxiliary space.

You are right. Nice catch.

Comment 20 Build Bot 2017-11-09 17:30:56 PST

Attachment 326517 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:350:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:351:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:352:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:353:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 21 Caio Lima 2017-11-10 15:24:06 PST

Created attachment 326639 [details]
Patch

Comment 22 Build Bot 2017-11-10 15:26:05 PST

Attachment 326639 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:350:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:351:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:352:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:353:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:195:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 23 Caio Lima 2017-11-10 15:28:19 PST

(In reply to Caio Lima from comment #21)
> Created attachment 326639 [details]
> Patch

This Patch is fixing some bugs I found on parseInt to handle "0b", "0o" and "0x" cases.

Comment 24 Caio Lima 2017-11-11 14:01:05 PST

Created attachment 326697 [details]
Patch

Comment 25 Build Bot 2017-11-11 14:03:29 PST

Attachment 326697 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:351:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:352:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:353:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:354:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:355:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:180:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:180:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:196:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 26 Caio Lima 2017-11-11 14:18:29 PST

(In reply to Caio Lima from comment #24)
> Created attachment 326697 [details]
> Patch

In this version I'm adding BigInt type on required primitives to allow BigInt be used as ObjectKey. Also I added BigIntObject to allow ToObject abstract operation. New tests were added.

Comment 27 Caio Lima 2017-11-15 14:19:19 PST

Ping review

Comment 28 Caio Lima 2017-11-15 18:25:37 PST

Created attachment 327042 [details]
Patch

Comment 29 Caio Lima 2017-11-15 18:27:01 PST

(In reply to Caio Lima from comment #28)
> Created attachment 327042 [details]
> Patch

This Patch is creating more tests cases on constructor and fixing errors found on these  cases

Comment 30 Caio Lima 2017-11-17 19:30:52 PST

I think this current Patch is becoming super huge and decided to split it up with BigInt literal support. So now this bug is dependent of https://bugs.webkit.org/show_bug.cgi?id=179000 and the implementation there contains JSBigInt.h and JSBitInt.cpp to make review easier

Comment 31 Caio Lima 2017-12-18 19:20:25 PST

Created attachment 329726 [details]
Patch

Comment 32 Yusuke Suzuki 2017-12-21 11:45:24 PST

Comment on attachment 329726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329726&action=review

r=me with comments.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:20
> + */

I think new code in WebKit uses 2-clauses BSD license.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:46
> +const ClassInfo BigIntConstructor::s_info = { "Function", &InternalFunction::s_info, &bigIntConstructorTable, nullptr, CREATE_METHOD_TABLE(BigIntConstructor) };

`&Base::s_info` is better than `&InternalFunction::s_info`.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:79
> +    if (argument.isInt32())
> +        isInteger = true;

Let's use early returns. Then, we do not need to have `bool isInteger`.
Like,

if (argument.isInt32())
    return true;

if (!argument.isDouble())
    return false;

...

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:103
> +        JSBigInt* result = JSBigInt::createFrom(vm, argument.asBoolean());
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +
> +        return JSValue::encode(result);

Let's just return JSBigInt::createFrom result directly. Like,

if (argument.isBoolean()) {
    scope.release();
    return JSValue::encode(JSBigInt::createFrom(vm, argument.asBoolean()));
}

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:107
> +        return throwVMTypeError(&state, scope);

Let's add an error message.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:116
> +        JSBigInt* bigInt = JSBigInt::parseInt(&state, view);
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +
> +        if (!bigInt)
> +            return throwVMError(&state, scope, createSyntaxError(&state, "Failed to parse String to BigInt"));

Why not throwing SyntaxError inside parseInt? Then, we do not need to have

if (!bigInt)
    ...

here. And we can just write this code as,

return JSBigInt::parseInt(&state, view);

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:120
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    return result;

Let's return `toStringView()` result directly.

scope.release();
return toStringView(...);

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:129
> +static EncodedJSValue JSC_HOST_CALL constructBigIntConstructor(ExecState* state)
> +{
> +    VM& vm = state->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +
> +    return throwVMTypeError(state, scope);
> +}

If you do not have [[Construct]] in BigInt, we should not define this function. Just passing `nullptr` to InternalFunction constructor. (See SymbolConstructor implementation).

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:147
> +            JSBigInt* result = JSBigInt::createFrom(vm, primitive.asInt32());
> +            RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +            return JSValue::encode(result);

Return `JSValue::encode(JSBigInt::createFrom(...))` directly.

scope.release();
return JSValue::encode(JSBigInt::createFrom(...));

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:153
> +            JSBigInt* result = JSBigInt::createFrom(vm, primitive.asUInt32());
> +            RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +            return JSValue::encode(result);

Ditto.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:158
> +        JSBigInt* result = JSBigInt::createFrom(vm, static_cast<int64_t>(primitive.asDouble()));
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +        return JSValue::encode(result);

Ditto.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:176
> +EncodedJSValue JSC_HOST_CALL bigIntConstructorFuncAsUintN(ExecState*)
> +{
> +    CRASH();
> +    return JSValue::encode(JSValue());
> +}
> +
> +EncodedJSValue JSC_HOST_CALL bigIntConstructorFuncAsIntN(ExecState*)
> +{
> +    CRASH();
> +    return JSValue::encode(JSValue());
> +}

Let's add a FIXME comment with URL.

> Source/JavaScriptCore/runtime/BigIntConstructor.h:19
> + */

Ditto

> Source/JavaScriptCore/runtime/BigIntConstructor.h:32
> +    typedef InternalFunction Base;

Use `using` in new code.

> Source/JavaScriptCore/runtime/BigIntObject.cpp:36
> +const ClassInfo BigIntObject::s_info = { "BigInt", &JSWrapperObject::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(BigIntObject) };

Use `&Base::s_info` instead.

> Source/JavaScriptCore/runtime/BigIntObject.h:20
> + */

Ditto

> Source/JavaScriptCore/runtime/BigIntObject.h:31
> +    typedef JSWrapperObject Base;

Use `using` instead.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:20
> + */

Ditto

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:73
> +static ALWAYS_INLINE bool toThisBigIntValue(VM& vm, JSValue thisValue, JSBigInt** x)

I think using `jsDynamicCast`'s idiom is preferable to JSC code.
Let's return `JSBigInt*`. If we cannot find it, just return nullptr.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> +    JSValue radixValue = state->argument(0);
> +    int32_t radix;
> +    if (radixValue.isInt32())
> +        radix = radixValue.asInt32();
> +    else if (radixValue.isUndefined())
> +        radix = 10;
> +    else
> +        radix = static_cast<int32_t>(radixValue.toInteger(state)); // nan -> 0
> +
> +    return radix;

Let's use early return.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:112
> +    JSBigInt* value = nullptr;
> +    if (!toThisBigIntValue(vm, state->thisValue(), &value))
> +        return throwVMTypeError(state, scope, ASCIILiteral("'this' value must be a BigInt or BigIntObject"));

Then, we can write this as,

JSBigInt* value = toThisBigIntValue(vm, state->thisValue());
if (!value)
    return throwVMTypeError(state, scope, ASCIILiteral("'this' value must be a BigInt or BigIntObject"));

> Source/JavaScriptCore/runtime/BigIntPrototype.h:19
> + */

Ditto

> Source/JavaScriptCore/runtime/BigIntPrototype.h:29
> +    typedef JSNonFinalObject Base;

Use `using` in new code.

> Source/JavaScriptCore/runtime/IntlCollator.cpp:263
> +    auto result = resolveLocale(state, availableLocales, requestedLocales, opt, relevantCollatorExtensionKeys, WTF_ARRAY_LENGTH(relevantCollatorExtensionKeys), localeData);

I think this file is not related to this patch.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:197
> +    auto result = resolveLocale(state, availableLocales, requestedLocales, opt, relevantNumberExtensionKeys, WTF_ARRAY_LENGTH(relevantNumberExtensionKeys), IntlNFInternal::localeData);

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:124
> +        bigInt->setDigit(0, static_cast<Digit>(-1 * tempValue));

Let's just write `bigInt->setDigit(0, static_cast<Digit>(-1 * static_cast<int64_t>(value)));`

> Source/JavaScriptCore/runtime/JSBigInt.cpp:153
> +            uint64_t tempValue = static_cast<uint64_t>(-(value + 1)) + 1;
> +            bigInt->setDigit(0, static_cast<Digit>(tempValue));

Let's just write `bigInt->setDigit(0, static_cast<Digit>(static_cast<uint64_t>(-(value + 1)) + 1)));`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:173
> +        uint64_t tempValue;
> +        bool sign = false;
> +        if (value < 0) {
> +            tempValue = static_cast<uint64_t>(-(value + 1)) + 1;
> +            sign = true;
> +        } else
> +            tempValue = value;
> +        
> +        Digit lowBits  = static_cast<Digit>(value & 0xffffffff);
> +        Digit highBits = static_cast<Digit>((value >> 32) & 0xffffffff);
> +        
> +        bigInt->setDigit(0, lowBits);
> +        bigInt->setDigit(1, highBits);
> +        bigInt->setSign(sign);

I think this has a bug since nobody uses tempValue. Could you add a test for this?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:735
> +        if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'b'))
> +            return parseInt(exec, vm, data, length, p + 2, 2, false);
> +        
> +        if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'x'))
> +            return parseInt(exec, vm, data, length, p + 2, 16, false);
> +        
> +        if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'o'))
> +            return parseInt(exec, vm, data, length, p + 2, 8, false);

Let's combine this `data[p] == '0'` check.

> Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:28
> +#include <mutex>

Why is this necessary?

Comment 33 Caio Lima 2017-12-23 13:17:14 PST

Comment on attachment 329726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329726&action=review

Thank you very much for the review!

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:20
>> + */
> 
> I think new code in WebKit uses 2-clauses BSD license.

Oops. Thx.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:46
>> +const ClassInfo BigIntConstructor::s_info = { "Function", &InternalFunction::s_info, &bigIntConstructorTable, nullptr, CREATE_METHOD_TABLE(BigIntConstructor) };
> 
> `&Base::s_info` is better than `&InternalFunction::s_info`.

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:79
>> +        isInteger = true;
> 
> Let's use early returns. Then, we do not need to have `bool isInteger`.
> Like,
> 
> if (argument.isInt32())
>     return true;
> 
> if (!argument.isDouble())
>     return false;
> 
> ...

Oops. Changed.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:103
>> +        return JSValue::encode(result);
> 
> Let's just return JSBigInt::createFrom result directly. Like,
> 
> if (argument.isBoolean()) {
>     scope.release();
>     return JSValue::encode(JSBigInt::createFrom(vm, argument.asBoolean()));
> }

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:107
>> +        return throwVMTypeError(&state, scope);
> 
> Let's add an error message.

Sure.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:116
>> +            return throwVMError(&state, scope, createSyntaxError(&state, "Failed to parse String to BigInt"));
> 
> Why not throwing SyntaxError inside parseInt? Then, we do not need to have
> 
> if (!bigInt)
>     ...
> 
> here. And we can just write this code as,
> 
> return JSBigInt::parseInt(&state, view);

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:120
>> +    return result;
> 
> Let's return `toStringView()` result directly.
> 
> scope.release();
> return toStringView(...);

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:129
>> +}
> 
> If you do not have [[Construct]] in BigInt, we should not define this function. Just passing `nullptr` to InternalFunction constructor. (See SymbolConstructor implementation).

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:147
>> +            return JSValue::encode(result);
> 
> Return `JSValue::encode(JSBigInt::createFrom(...))` directly.
> 
> scope.release();
> return JSValue::encode(JSBigInt::createFrom(...));

Done.

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:176
>> +}
> 
> Let's add a FIXME comment with URL.

Done.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:173
>> +        bigInt->setSign(sign);
> 
> I think this has a bug since nobody uses tempValue. Could you add a test for this?

I added "BigInt(-4503599627370496);" into stress/big-int-constructor.js. This test is 32-bit specific.

>> Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:28
>> +#include <mutex>
> 
> Why is this necessary?

It was missing by the time I wrote this Patch. Now it is there, but basically it was missing and "std::once_flag" and "std::call_once" were generating compilation error.

Comment 34 Caio Lima 2017-12-24 14:54:06 PST

Created attachment 330175 [details]
Patch

Comment 35 EWS Watchlist 2017-12-24 16:55:21 PST

Comment on attachment 330175 [details]
Patch

Attachment 330175 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5821633

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

Comment 36 EWS Watchlist 2017-12-24 16:55:22 PST

Created attachment 330176 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

Comment 37 Caio Lima 2017-12-25 09:28:17 PST

Created attachment 330184 [details]
Patch

Comment 38 Caio Lima 2017-12-27 07:53:51 PST

Created attachment 330216 [details]
Patch

Requesting new Batch of review because I changed the implementation of JSBigInt::parseInt

Comment 39 Yusuke Suzuki 2017-12-28 08:37:39 PST

Comment on attachment 330216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330216&action=review

Nice.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:100
> +    return static_cast<int32_t>(radixValue.toInteger(state)); // nan -> 0

It seems that this is not correct. What happens if radiValue.toInteger() double is larger than int32_t?
And in Windows, static_cast<int32_t>(infinity double) is not allowed.
I'm not sure `NaN` -> `0` is guaranteed in the spec. Can you check this?

[1]: https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:121
> +    String resultString = value->toString(*state, radix);

This would raise errors. I think we should move `scope.release()` just before .... v.
And let's insert `RETURN_IF_EXCEPTION(scope, encodedJSValue());` after `toString`.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:125
> +    if (resultString.length() == 1)
> +        return JSValue::encode(vm.smallStrings.singleCharacterString(resultString[0]));
> +
> +    return JSValue::encode(jsNontrivialString(&vm, resultString));

Here.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:139
> +    return JSValue::encode(JSValue());

I think it is incorrect. It returns JSEmpty, which should not be user-observable.
Can you add a test for `bigIntValueOf.call(nonBigInt)`?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:149
> +    JSBigInt* bigInt;

Drop this.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:151
> +        bigInt = createWithLength(vm, 1);

Make this as `JSBigInt* bigInt = ...;`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:159
> +        }

Let's `return bigInt;` here.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:161
> +        bigInt = createWithLength(vm, 2);

Make this `JSBigInt* bigInt = `.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:176
> +        bigInt->setSign(sign);

And move these statements from this brace.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:215
> +        return parseInt(state, s, s.characters8());
> +    return parseInt(state, s, s.characters16());

Let's pass length.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:721
> +JSBigInt* JSBigInt::parseInt(ExecState* state, StringView s, CharType*  data)

Let's take length instead of StringView.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:771
> +        if (state)
> +            throwVMError(state, scope, createSyntaxError(state, "Failed to parse String to BigInt"));
> +

Let's insert ASSERT(state); instead of `if (state)`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:816
> +    if (state)
> +        throwVMError(state, scope, createSyntaxError(state, "Failed to parse String to BigInt"));

Let's insert ASSERT(state); instead of `if (state)`.

Comment 40 Caio Lima 2017-12-28 09:50:16 PST

Comment on attachment 330216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330216&action=review

Thanks for the review!

>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:100
>> +    return static_cast<int32_t>(radixValue.toInteger(state)); // nan -> 0
> 
> It seems that this is not correct. What happens if radiValue.toInteger() double is larger than int32_t?
> And in Windows, static_cast<int32_t>(infinity double) is not allowed.
> I'm not sure `NaN` -> `0` is guaranteed in the spec. Can you check this?
> 
> [1]: https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring

```NaN -> 0``` is in spec https://tc39.github.io/ecma262/#sec-tointeger

This also seem wrong to me. This code was originally from Source/JavaScriptCore/runtime/NumberPrototype.cpp. It is potentially a bug there as well.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:771
>> +
> 
> Let's insert ASSERT(state); instead of `if (state)`.

```state``` here can be nullptr, because this function is also called from BytecodeGenerator.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:816
>> +        throwVMError(state, scope, createSyntaxError(state, "Failed to parse String to BigInt"));
> 
> Let's insert ASSERT(state); instead of `if (state)`.

ditto

Comment 41 Yusuke Suzuki 2017-12-28 10:01:45 PST

Comment on attachment 330216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330216&action=review

>>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:100
>>> +    return static_cast<int32_t>(radixValue.toInteger(state)); // nan -> 0
>> 
>> It seems that this is not correct. What happens if radiValue.toInteger() double is larger than int32_t?
>> And in Windows, static_cast<int32_t>(infinity double) is not allowed.
>> I'm not sure `NaN` -> `0` is guaranteed in the spec. Can you check this?
>> 
>> [1]: https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring
> 
> ```NaN -> 0``` is in spec https://tc39.github.io/ecma262/#sec-tointeger
> 
> This also seem wrong to me. This code was originally from Source/JavaScriptCore/runtime/NumberPrototype.cpp. It is potentially a bug there as well.

Ah, no. I mean that is `static_cast<int32_t>(NaN)` => 0 is specified in C++ spec? I don't think so.

Comment 42 Yusuke Suzuki 2017-12-28 10:03:11 PST

Comment on attachment 330216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330216&action=review

>>> Source/JavaScriptCore/runtime/JSBigInt.cpp:771
>>> +
>> 
>> Let's insert ASSERT(state); instead of `if (state)`.
> 
> ```state``` here can be nullptr, because this function is also called from BytecodeGenerator.

No. At that time, we should not throw any errors. So this path is not taken. No?

>>> Source/JavaScriptCore/runtime/JSBigInt.cpp:816
>>> +        throwVMError(state, scope, createSyntaxError(state, "Failed to parse String to BigInt"));
>> 
>> Let's insert ASSERT(state); instead of `if (state)`.
> 
> ditto

Explained in the above comment.

Comment 43 Caio Lima 2017-12-28 11:40:35 PST

Comment on attachment 330216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330216&action=review

>>>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:100
>>>> +    return static_cast<int32_t>(radixValue.toInteger(state)); // nan -> 0
>>> 
>>> It seems that this is not correct. What happens if radiValue.toInteger() double is larger than int32_t?
>>> And in Windows, static_cast<int32_t>(infinity double) is not allowed.
>>> I'm not sure `NaN` -> `0` is guaranteed in the spec. Can you check this?
>>> 
>>> [1]: https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring
>> 
>> ```NaN -> 0``` is in spec https://tc39.github.io/ecma262/#sec-tointeger
>> 
>> This also seem wrong to me. This code was originally from Source/JavaScriptCore/runtime/NumberPrototype.cpp. It is potentially a bug there as well.
> 
> Ah, no. I mean that is `static_cast<int32_t>(NaN)` => 0 is specified in C++ spec? I don't think so.

Sorry for misunderstanding. According JS spec, ```radixValue.toInteger(state)``` can't return NaN, because ```ToNumber(v)``` abstract operation returns ```+0``` if ```v == NaN```.
However, when ```ToInteger``` is returning a value out of int32_t range, the cast is resulting into "-2147483648". I'm not sure if such behavior is defined in C++ spec. Do you have any knowledge about it?

>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:139
>> +    return JSValue::encode(JSValue());
> 
> I think it is incorrect. It returns JSEmpty, which should not be user-observable.
> Can you add a test for `bigIntValueOf.call(nonBigInt)`?

Nice Catch.

>>>> Source/JavaScriptCore/runtime/JSBigInt.cpp:771
>>>> +
>>> 
>>> Let's insert ASSERT(state); instead of `if (state)`.
>> 
>> ```state``` here can be nullptr, because this function is also called from BytecodeGenerator.
> 
> No. At that time, we should not throw any errors. So this path is not taken. No?

Oh, you are right. I will apply that.

Comment 44 Caio Lima 2017-12-28 16:51:11 PST

Created attachment 330236 [details]
Patch

Comment 45 Caio Lima 2017-12-28 18:50:13 PST

Created attachment 330239 [details]
Patch

Comment 46 Yusuke Suzuki 2017-12-30 10:36:01 PST

Comment on attachment 330239 [details]
Patch

r=me

Comment 47 Caio Lima 2018-01-02 14:14:18 PST

Created attachment 330334 [details]
Patch for landing

Comment 48 Caio Lima 2018-01-02 14:16:54 PST

Created attachment 330335 [details]
Patch for landing

Comment 49 Caio Lima 2018-01-02 15:05:24 PST

Created attachment 330342 [details]
Patch for landing

Comment 50 WebKit Commit Bot 2018-01-02 15:38:40 PST

Comment on attachment 330342 [details]
Patch for landing

Clearing flags on attachment: 330342

Committed r226338: <https://trac.webkit.org/changeset/226338>

Comment 51 WebKit Commit Bot 2018-01-02 15:38:42 PST

All reviewed patches have been landed.  Closing bug.

Comment 52 Radar WebKit Bug Importer 2018-01-02 15:39:27 PST

<rdar://problem/36264650>

Comment 53 Joseph Pecoraro 2018-01-02 16:51:10 PST

Comment on attachment 330342 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=330342&action=review

Nice! Some follow-up comments, nothing seriously.

Out of curiosity, should we have any tests for Math functions with BigInt values? For example `isFinite`, `isNaN`, etc?

> JSTests/stress/big-int-constructor-properties.js:9
> +    let p = Object.getOwnPropertyDescriptor(BigInt, "asUintN");

Check the function name and length as well?

> JSTests/stress/big-int-constructor-properties.js:17
> +    let p = Object.getOwnPropertyDescriptor(BigInt, "asIntN");

Check the function name and length as well?

> JSTests/stress/big-int-constructor.js:79
> +n = BigInt("0b10");
> +assert(n.toString() === "2");
> +
> +n = BigInt("0b10");
> +assert(n.toString() === "2");

Same test twice? Maybe convert the second to 0b010 to test a leading 0?

> JSTests/stress/big-int-constructor.js:95
> +n = BigInt("0B10");
> +assert(n.toString() === "2");
> +
> +n = BigInt("0B10");
> +assert(n.toString() === "2");

Same test twice? Maybe convert the second to 0B010 to test a leading 0?

> JSTests/stress/big-int-constructor.js:181
> +// Objects

I didn't see any tests for a BigInt constructed with a BigInt. For example:

    n = BigInt(123n);
    assert(n.toString() === "123");

    n = BigInt(BigInt(456));
    assert(n.toString() === "456");

> JSTests/stress/big-int-prototype-properties.js:11
> +    assert(p.enumerable === false);

Same thing for testing function name and length.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1140
> +		861816771FB7924200ECC4EC /* BigIntObject.h in Headers */ = {isa = PBXBuildFile; fileRef = 861816761FB7922F00ECC4EC /* BigIntObject.h */; settings = {ATTRIBUTES = (Private, ); }; };

Does this need to be Private? I don't think it needs to be.

Private means this shows up in the PrivateHeaders directory of the JavaScriptCore.framework. This is normally done if the resource (BigIntObject.h) needs to be accessed by another target outside of the target it builds (such as the jsc command line tool, or WebCore). That doesn't appear to be the case here since this file is only included by JavaScriptCore cpp files.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1154
> +		86976E5E1FA3E8B600E7C4E1 /* BigIntPrototype.h in Headers */ = {isa = PBXBuildFile; fileRef = 86976E5A1FA3758A00E7C4E1 /* BigIntPrototype.h */; settings = {ATTRIBUTES = (Private, ); }; };
> +		86976E5F1FA3E8BC00E7C4E1 /* BigIntConstructor.h in Headers */ = {isa = PBXBuildFile; fileRef = 86976E571FA3754000E7C4E1 /* BigIntConstructor.h */; settings = {ATTRIBUTES = (Private, ); }; };

Ditto.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:60
> +static EncodedJSValue JSC_HOST_CALL callBigIntConstructor(ExecState*);

Should this go up with the other host call forward declarations (line 40)?

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:74
> +    putDirect(vm, vm.propertyNames->name, jsNontrivialString(&vm, String(ASCIILiteral("BigInt"))), PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum);

Is this line necessary? I think it can be dropped.

It seems `Base::finishCreation(...)` should handle this because it passes the proper className (BigIntPrototype::info()->className is "BigInt") and has uses the default NameVisibility::Visible. Other JavaScript FooConstructor classes don't explicitly set the name property.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:128
> +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("Not safe integer")));

Hmm, this error message could be more user friendly.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:114
>  
> +

Style: Only 1 newline needed between member functions.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:189
> +    bigInt->setDigit(0, static_cast<Digit>(value));

The spec converts a boolean to exactly 0 or 1n:
https://tc39.github.io/proposal-bigint/#sec-to-bigint

Is it possible that the bitwise representation of a truthy `bool`, when casted to Digit (uintptr_t) to be a value other than 0 or 1? Should be cautious here and write out the two sides?

    bigInt->setDigit(0, value ? static_cast<Digit>(1) : static_cast<Digit>(0));

> Source/JavaScriptCore/runtime/JSBigInt.cpp:730
> +    // Check Radix from frist characters

Typo: "frist" => "first"
Style: Comments are complete sentences and should end in a period.

Comment 54 Caio Lima 2018-01-02 18:10:45 PST

(In reply to Joseph Pecoraro from comment #53)

Thanks for the review and comments!

> Out of curiosity, should we have any tests for Math functions with BigInt
> values? For example `isFinite`, `isNaN`, etc?

I think we should. I'm going to do that.

> > JSTests/stress/big-int-constructor-properties.js:9
> > +    let p = Object.getOwnPropertyDescriptor(BigInt, "asUintN");
> 
> Check the function name and length as well?

Yes.

> > JSTests/stress/big-int-constructor-properties.js:17
> > +    let p = Object.getOwnPropertyDescriptor(BigInt, "asIntN");
> 
> Check the function name and length as well?

Ditto.

> > JSTests/stress/big-int-constructor.js:79
> > +n = BigInt("0b10");
> > +assert(n.toString() === "2");
> > +
> > +n = BigInt("0b10");
> > +assert(n.toString() === "2");
> 
> Same test twice? Maybe convert the second to 0b010 to test a leading 0?

Nice Catch. I will follow your suggestion.

> > JSTests/stress/big-int-constructor.js:95
> > +n = BigInt("0B10");
> > +assert(n.toString() === "2");
> > +
> > +n = BigInt("0B10");
> > +assert(n.toString() === "2");
> 
> Same test twice? Maybe convert the second to 0B010 to test a leading 0?
> 
> > JSTests/stress/big-int-constructor.js:181
> > +// Objects
> 
> I didn't see any tests for a BigInt constructed with a BigInt. For example:
> 
>     n = BigInt(123n);
>     assert(n.toString() === "123");
> 
>     n = BigInt(BigInt(456));
>     assert(n.toString() === "456");

Sure.

> > JSTests/stress/big-int-prototype-properties.js:11
> > +    assert(p.enumerable === false);
> 
> Same thing for testing function name and length.
> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1140
> > +		861816771FB7924200ECC4EC /* BigIntObject.h in Headers */ = {isa = PBXBuildFile; fileRef = 861816761FB7922F00ECC4EC /* BigIntObject.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Does this need to be Private? I don't think it needs to be.

Oops.

> 
> > Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:1154
> > +		86976E5E1FA3E8B600E7C4E1 /* BigIntPrototype.h in Headers */ = {isa = PBXBuildFile; fileRef = 86976E5A1FA3758A00E7C4E1 /* BigIntPrototype.h */; settings = {ATTRIBUTES = (Private, ); }; };
> > +		86976E5F1FA3E8BC00E7C4E1 /* BigIntConstructor.h in Headers */ = {isa = PBXBuildFile; fileRef = 86976E571FA3754000E7C4E1 /* BigIntConstructor.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Ditto.
> 
> > Source/JavaScriptCore/runtime/BigIntConstructor.cpp:60
> > +static EncodedJSValue JSC_HOST_CALL callBigIntConstructor(ExecState*);
> 
> Should this go up with the other host call forward declarations (line 40)?

Sure.

> > Source/JavaScriptCore/runtime/BigIntConstructor.cpp:74
> > +    putDirect(vm, vm.propertyNames->name, jsNontrivialString(&vm, String(ASCIILiteral("BigInt"))), PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum);
> 
> Is this line necessary? I think it can be dropped.
> 
> It seems `Base::finishCreation(...)` should handle this because it passes
> the proper className (BigIntPrototype::info()->className is "BigInt") and
> has uses the default NameVisibility::Visible. Other JavaScript
> FooConstructor classes don't explicitly set the name property.
> 
> > Source/JavaScriptCore/runtime/BigIntConstructor.cpp:128
> > +            return throwVMError(state, scope, createRangeError(state, ASCIILiteral("Not safe integer")));
> 
> Hmm, this error message could be more user friendly.

Agreed.

> > Source/JavaScriptCore/runtime/JSBigInt.cpp:114
> >  
> > +
> 
> Style: Only 1 newline needed between member functions.
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:189
> > +    bigInt->setDigit(0, static_cast<Digit>(value));
> 
> The spec converts a boolean to exactly 0 or 1n:
> https://tc39.github.io/proposal-bigint/#sec-to-bigint
> 
> Is it possible that the bitwise representation of a truthy `bool`, when
> casted to Digit (uintptr_t) to be a value other than 0 or 1? Should be
> cautious here and write out the two sides?
> 
>     bigInt->setDigit(0, value ? static_cast<Digit>(1) :
> static_cast<Digit>(0));

I agree.

> > Source/JavaScriptCore/runtime/JSBigInt.cpp:730
> > +    // Check Radix from frist characters
> 
> Typo: "frist" => "first"
> Style: Comments are complete sentences and should end in a period.

Oops.