WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
175359
[ESNext][BigInt] Implement BigIntConstructor and BigIntPrototype
https://bugs.webkit.org/show_bug.cgi?id=175359
Summary
[ESNext][BigInt] Implement BigIntConstructor and BigIntPrototype
Caio Lima
Reported
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
Attachments
WIP - It starts
(24.19 KB, patch)
2017-10-22 16:51 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
WIP - Added parseInt
(32.98 KB, patch)
2017-10-24 20:11 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
RFC - Patch proposal
(56.85 KB, patch)
2017-10-27 14:01 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(83.04 KB, patch)
2017-10-30 19:27 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(82.95 KB, patch)
2017-11-02 10:04 PDT
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(81.73 KB, patch)
2017-11-09 07:53 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(81.80 KB, patch)
2017-11-09 17:27 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(82.77 KB, patch)
2017-11-10 15:24 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(106.70 KB, patch)
2017-11-11 14:01 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(108.04 KB, patch)
2017-11-15 18:25 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(76.63 KB, patch)
2017-12-18 19:20 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(76.04 KB, patch)
2017-12-24 14:54 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
(2.66 MB, application/zip)
2017-12-24 16:55 PST
,
EWS Watchlist
no flags
Details
Patch
(76.04 KB, patch)
2017-12-25 09:28 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(73.07 KB, patch)
2017-12-27 07:53 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(75.94 KB, patch)
2017-12-28 16:51 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch
(75.96 KB, patch)
2017-12-28 18:50 PST
,
Caio Lima
ysuzuki
: review+
Details
Formatted Diff
Diff
Patch for landing
(73.28 KB, patch)
2018-01-02 14:14 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch for landing
(73.27 KB, patch)
2018-01-02 14:16 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Patch for landing
(73.27 KB, patch)
2018-01-02 15:05 PST
,
Caio Lima
no flags
Details
Formatted Diff
Diff
Show Obsolete
(19)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
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.
Caio Lima
Comment 2
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.
Caio Lima
Comment 3
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.
Caio Lima
Comment 4
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.
Build Bot
Comment 5
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.
Caio Lima
Comment 6
2017-10-30 19:27:45 PDT
Created
attachment 325411
[details]
Patch
Build Bot
Comment 7
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.
Saam Barati
Comment 8
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`
Yusuke Suzuki
Comment 9
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.
Yusuke Suzuki
Comment 10
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.
Caio Lima
Comment 11
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.
Caio Lima
Comment 12
2017-11-02 10:04:18 PDT
Created
attachment 325725
[details]
Patch
Build Bot
Comment 13
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.
Caio Lima
Comment 14
2017-11-07 14:38:01 PST
Ping review
Caio Lima
Comment 15
2017-11-09 07:53:13 PST
Created
attachment 326447
[details]
Patch
Build Bot
Comment 16
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.
Yusuke Suzuki
Comment 17
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.
Caio Lima
Comment 18
2017-11-09 17:27:43 PST
Created
attachment 326517
[details]
Patch
Caio Lima
Comment 19
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.
Build Bot
Comment 20
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.
Caio Lima
Comment 21
2017-11-10 15:24:06 PST
Created
attachment 326639
[details]
Patch
Build Bot
Comment 22
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.
Caio Lima
Comment 23
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.
Caio Lima
Comment 24
2017-11-11 14:01:05 PST
Created
attachment 326697
[details]
Patch
Build Bot
Comment 25
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.
Caio Lima
Comment 26
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.
Caio Lima
Comment 27
2017-11-15 14:19:19 PST
Ping review
Caio Lima
Comment 28
2017-11-15 18:25:37 PST
Created
attachment 327042
[details]
Patch
Caio Lima
Comment 29
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
Caio Lima
Comment 30
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
Caio Lima
Comment 31
2017-12-18 19:20:25 PST
Created
attachment 329726
[details]
Patch
Yusuke Suzuki
Comment 32
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?
Caio Lima
Comment 33
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.
Caio Lima
Comment 34
2017-12-24 14:54:06 PST
Created
attachment 330175
[details]
Patch
EWS Watchlist
Comment 35
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
EWS Watchlist
Comment 36
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
Caio Lima
Comment 37
2017-12-25 09:28:17 PST
Created
attachment 330184
[details]
Patch
Caio Lima
Comment 38
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
Yusuke Suzuki
Comment 39
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)`.
Caio Lima
Comment 40
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
Yusuke Suzuki
Comment 41
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.
Yusuke Suzuki
Comment 42
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.
Caio Lima
Comment 43
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.
Caio Lima
Comment 44
2017-12-28 16:51:11 PST
Created
attachment 330236
[details]
Patch
Caio Lima
Comment 45
2017-12-28 18:50:13 PST
Created
attachment 330239
[details]
Patch
Yusuke Suzuki
Comment 46
2017-12-30 10:36:01 PST
Comment on
attachment 330239
[details]
Patch r=me
Caio Lima
Comment 47
2018-01-02 14:14:18 PST
Created
attachment 330334
[details]
Patch for landing
Caio Lima
Comment 48
2018-01-02 14:16:54 PST
Created
attachment 330335
[details]
Patch for landing
Caio Lima
Comment 49
2018-01-02 15:05:24 PST
Created
attachment 330342
[details]
Patch for landing
WebKit Commit Bot
Comment 50
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
>
WebKit Commit Bot
Comment 51
2018-01-02 15:38:42 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 52
2018-01-02 15:39:27 PST
<
rdar://problem/36264650
>
Joseph Pecoraro
Comment 53
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.
Caio Lima
Comment 54
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug