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
WIP - Added parseInt (32.98 KB, patch)
2017-10-24 20:11 PDT, Caio Lima
no flags
RFC - Patch proposal (56.85 KB, patch)
2017-10-27 14:01 PDT, Caio Lima
no flags
Patch (83.04 KB, patch)
2017-10-30 19:27 PDT, Caio Lima
no flags
Patch (82.95 KB, patch)
2017-11-02 10:04 PDT, Caio Lima
no flags
Patch (81.73 KB, patch)
2017-11-09 07:53 PST, Caio Lima
no flags
Patch (81.80 KB, patch)
2017-11-09 17:27 PST, Caio Lima
no flags
Patch (82.77 KB, patch)
2017-11-10 15:24 PST, Caio Lima
no flags
Patch (106.70 KB, patch)
2017-11-11 14:01 PST, Caio Lima
no flags
Patch (108.04 KB, patch)
2017-11-15 18:25 PST, Caio Lima
no flags
Patch (76.63 KB, patch)
2017-12-18 19:20 PST, Caio Lima
no flags
Patch (76.04 KB, patch)
2017-12-24 14:54 PST, Caio Lima
no flags
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
Patch (76.04 KB, patch)
2017-12-25 09:28 PST, Caio Lima
no flags
Patch (73.07 KB, patch)
2017-12-27 07:53 PST, Caio Lima
no flags
Patch (75.94 KB, patch)
2017-12-28 16:51 PST, Caio Lima
no flags
Patch (75.96 KB, patch)
2017-12-28 18:50 PST, Caio Lima
ysuzuki: review+
Patch for landing (73.28 KB, patch)
2018-01-02 14:14 PST, Caio Lima
no flags
Patch for landing (73.27 KB, patch)
2018-01-02 14:16 PST, Caio Lima
no flags
Patch for landing (73.27 KB, patch)
2018-01-02 15:05 PST, Caio Lima
no flags
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
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
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
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
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
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
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
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
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
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
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
Caio Lima
Comment 45 2017-12-28 18:50:13 PST
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
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.