Bug 179000 – [ESNext][BigInt] Implement BigInt literals and JSBigInt

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

Support syntax for 

```
let a = 10n;
```

[Caio Lima]

Created attachment 327296 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:197:  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 48 files


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

[Caio Lima]

Created attachment 327297 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:197:  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 47 files


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

[Caio Lima]

Created attachment 327298 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:197:  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 47 files


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

[Caio Lima]

Created attachment 327299 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:197:  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 47 files


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

[Caio Lima]

Created attachment 327301 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:197:  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 47 files


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

[Yusuke Suzuki]

Comment on attachment 327301 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:4
> +        [ESNext][BigInt] Implement JSBigInt, BigIntConstructor and BigIntPrototype
> +        https://bugs.webkit.org/show_bug.cgi?id=179000

Fix the title.

> Source/JavaScriptCore/parser/Lexer.cpp:1503
> +ALWAYS_INLINE bool Lexer<T>::parseHex(double& returnValue)

Let's return enum, Failure, Number, and BigInt.

> Source/JavaScriptCore/parser/Lexer.cpp:1515
> +    if (maximumDigits >= 0 && UNLIKELY(Options::useBigInt() && (m_current | 0x20) != 'n')) {

This `UNLIKELY()` seems wrong. Once `Options::useBigInt()` becomes true, this likes BigInt than usual hex value.

> Source/JavaScriptCore/parser/Lexer.cpp:1543
>  template <typename T>

Ditto

> Source/JavaScriptCore/parser/Lexer.cpp:1561
> +    if (!isASCIIDigit(m_current) && digit >= 0 && UNLIKELY((m_current | 0x20) != 'n')) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1604
> +    if (!isASCIIDigit(m_current) && digit >= 0 && UNLIKELY(Options::useBigInt() && (m_current | 0x20) != 'n')) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1623
> +    if (UNLIKELY((m_current | 0x20) == 'n'))
> +        return false;

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1651
> +        if (digit >= 0 && m_current != '.' && (m_current | 0x20) != 'e' && UNLIKELY(Options::useBigInt() && (m_current | 0x20) != 'n')) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2133
> +                Vector<LChar> completeIdent;
> +                completeIdent.append('0');
> +                completeIdent.append('x');
> +                completeIdent.append(m_buffer8.data(), m_buffer8.size());
> +                tokenData->ident = makeIdentifier(completeIdent.data(), completeIdent.size());

Instead of doing this, holding `radix` value and current m_buffer8's completeIdent in tokenData's BigInt fields should be clearer.
Then, when creating BigIntMap in bytecode generator, we also need to consider radix value.

> Source/JavaScriptCore/parser/Lexer.cpp:2164
> +            if (!parseBinary(tokenData->doubleValue) && UNLIKELY(Options::useBigInt() && (m_current | 0x20) == 'n')) {
> +                token = BIGINT;
> +                shift();
> +                Vector<LChar> completeIdent;
> +                completeIdent.append('0');
> +                completeIdent.append('b');
> +                completeIdent.append(m_buffer8.data(), m_buffer8.size());
> +                tokenData->ident = makeIdentifier(completeIdent.data(), completeIdent.size());
> +            };

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2195
> +            if (!parseOctal(tokenData->doubleValue) && UNLIKELY(Options::useBigInt() && (m_current | 0x20) == 'n')) {
> +                token = BIGINT;
> +                shift();
> +                Vector<LChar> completeIdent;
> +                completeIdent.append('0');
> +                completeIdent.append('o');
> +                completeIdent.append(m_buffer8.data(), m_buffer8.size());
> +                tokenData->ident = makeIdentifier(completeIdent.data(), completeIdent.size());
> +            }

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2222
>              if (!parseDecimal(tokenData->doubleValue)) {

Ditto

> Source/JavaScriptCore/parser/Lexer.cpp:2223
> +                if (UNLIKELY(Options::useBigInt() && (m_current | 0x20) == 'n')) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2226
> +                    token = BIGINT;
>                      shift();
> +                    tokenData->ident = makeIdentifier(m_buffer8.data(), m_buffer8.size());

Ditto.

> Source/JavaScriptCore/parser/NodeConstructors.h:96
> +        : ConstantNode(location, ResultType::stringType())

ResultType is not correct.

> Source/JavaScriptCore/parser/Nodes.h:341
> +        const Identifier& m_value;

Let's hold radix too.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:89
> +#define HAVE_TWODIGIT_T 1

Rename to HAVE_TODIGIT.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:92
> +#define HAVE_TWODIGIT_T 1

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:100
> +#if HAVE_TWODIGIT_T

Use `#if HAVE(TWODIGIT)`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:119
> +#if HAVE_TWODIGIT_T

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:136
> +#if HAVE_TWODIGIT_T

Ditto.

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:29
> +#include "BuiltinNames.h"

Why is it necessary?

> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:34
> +#include "JSModuleLoader.h"

Ditto.

> Tools/ChangeLog:3
> +        [ESNext][BigInt] Implement JSBigInt, BigIntConstructor and BigIntPrototype

Fix the title.

> JSTests/ChangeLog:3
> +        [ESNext][BigInt] Implement JSBigInt, BigIntConstructor and BigIntPrototype

Fix the title.

[Caio Lima]

Comment on attachment 327301 [details]
Patch

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

>> Source/JavaScriptCore/parser/Lexer.cpp:1503
>> +ALWAYS_INLINE bool Lexer<T>::parseHex(double& returnValue)
> 
> Let's return enum, Failure, Number, and BigInt.

Agreed.

>> Source/JavaScriptCore/parser/Lexer.cpp:1515
>> +    if (maximumDigits >= 0 && UNLIKELY(Options::useBigInt() && (m_current | 0x20) != 'n')) {
> 
> This `UNLIKELY()` seems wrong. Once `Options::useBigInt()` becomes true, this likes BigInt than usual hex value.

Done.

>> Source/JavaScriptCore/parser/Lexer.cpp:2133
>> +                tokenData->ident = makeIdentifier(completeIdent.data(), completeIdent.size());
> 
> Instead of doing this, holding `radix` value and current m_buffer8's completeIdent in tokenData's BigInt fields should be clearer.
> Then, when creating BigIntMap in bytecode generator, we also need to consider radix value.

Done as well

>> Source/JavaScriptCore/parser/NodeConstructors.h:96
>> +        : ConstantNode(location, ResultType::stringType())
> 
> ResultType is not correct.

Nice catch.

>> Source/JavaScriptCore/parser/Nodes.h:341
>> +        const Identifier& m_value;
> 
> Let's hold radix too.

Ok.

>> Source/JavaScriptCore/runtime/JSModuleRecord.cpp:29
>> +#include "BuiltinNames.h"
> 
> Why is it necessary?

No.

[Caio Lima]

Created attachment 327346 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:366:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:367:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:368:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:369:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:370:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:204:  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 46 files


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

[Yusuke Suzuki]

Comment on attachment 327346 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1004
> +        typedef std::pair<UniquedStringImpl*, uint8_t> BigIntMapEntry;

Use `using` instead of `typedef`

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1022
>          typedef HashMap<double, JSValue> NumberMap;
>          typedef HashMap<UniquedStringImpl*, JSString*, IdentifierRepHash> IdentifierStringMap;
> +        typedef HashMap<BigIntMapEntry, JSBigInt*, BigIntEntryHash> IdentifierBigIntMap;
>          typedef HashMap<Ref<TemplateRegistryKey>, RegisterID*> TemplateRegistryKeyMap;

Let's convert them to `using`.

> Source/JavaScriptCore/parser/Lexer.cpp:1515
> +    if (maximumDigits >= 0 && Options::useBigInt() && (m_current | 0x20) != 'n') {

This condition is wrong. If Options::useBigInt() is false, this condition never met.
And could you add a test to cover the above thing?
Ditto. And non-BigInt case should be handled as LIKELY in if-brace.

> Source/JavaScriptCore/parser/Lexer.cpp:1537
> +    if (UNLIKELY((m_current | 0x20) == 'n'))
> +        return BigInt;

Can we use BigInt without Options::useBigInt() check?

> Source/JavaScriptCore/parser/Lexer.cpp:1561
> +    if (!isASCIIDigit(m_current) && digit >= 0 && (m_current | 0x20) != 'n') {

Ditto. And non-BigInt case should be handled as LIKELY in if-brace.

> Source/JavaScriptCore/parser/Lexer.cpp:1575
> +    if ((m_current | 0x20) == 'n')
> +        return BigInt;

Ditto. And use UNLIEKLY

> Source/JavaScriptCore/parser/Lexer.cpp:1604
> +    if (!isASCIIDigit(m_current) && digit >= 0 && Options::useBigInt() && (m_current | 0x20) != 'n') {

Ditto. And non-BigInt case should be handled as LIKELY in if-brace.

> Source/JavaScriptCore/parser/Lexer.cpp:1618
> +    if ((m_current | 0x20) == 'n')
> +        return BigInt;

Ditto. And we should make it UNLIKELY.

> Source/JavaScriptCore/parser/Lexer.cpp:1651
> +        if (digit >= 0 && m_current != '.' && (m_current | 0x20) != 'e' && Options::useBigInt() && (m_current | 0x20) != 'n') {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1666
> +    if ((m_current | 0x20) == 'n')
> +        return BigInt;

Ditto. And we should make it UNLIKELY.

> Source/JavaScriptCore/parser/Lexer.cpp:2209
> +            if (parseOctal(tokenData->doubleValue) == NumberParseResult::Number) {

What happens if it returns BigInt?

> Source/JavaScriptCore/parser/Lexer.cpp:2215
> +        NumberParseResult parseResult;

Remove this.

> Source/JavaScriptCore/parser/Lexer.cpp:2217
> +            parseResult = parseDecimal(tokenData->doubleValue);

`NumberParseResult parseResult = parseDecimal(tokenData->doubleValue);` is better.

> Source/JavaScriptCore/parser/Lexer.h:176
> +    enum NumberParseResult {

Use `enum class`.

> Source/JavaScriptCore/parser/NodeConstructors.h:96
> +        : ConstantNode(location, ResultType::unknownType())

Let's add ResultType::bigIntType() and its handling in ResultType.h.

> Source/JavaScriptCore/parser/Parser.cpp:4496
> +        const Identifier* ident = m_token.m_data.ident;

Retrieve m_token.m_data.bigIntString.

> Source/JavaScriptCore/parser/ParserTokens.h:221
>      struct {
>          const Identifier* ident;
>          bool escaped;
> +        uint8_t radix;
>      };

We should add a new struct definition to this union for BigInt for readability.
Like,

struct {
    const Identifier* bigIntString;
    uint32_t radix;
};

> Source/JavaScriptCore/runtime/JSBigInt.cpp:90
> +typedef uint64_t TwoDigit;

Let's use `using`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:93
> +typedef __uint128_t TwoDigit;

Let's use `using`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:366
> +    0,   0,   32,  51,  64,  75,  83,  90,  96,  // 0..8

`//` should start with one space. Not two.

> Source/JavaScriptCore/runtime/JSBigInt.h:39
> +    typedef JSCell Base;

Use `using` instead of typedef.

> Source/JavaScriptCore/runtime/JSBigInt.h:40
> +    static const unsigned StructureFlags = Base::StructureFlags;

This is not necessary.

> Source/JavaScriptCore/runtime/JSBigInt.h:85
> +            bigInt->setDigit(0, static_cast<Digit>(-1 * value));

Consider INT32_MIN case. -INT32_MIN is not representable in int32_t. I think we should extend value to 64bit first and negate it.

> Source/JavaScriptCore/runtime/JSBigInt.h:114
> +            bigInt->setDigit(0, static_cast<Digit>(-1 * value));

Consider INT64_MIN case. -INT64_MIN is not representable in int64_t. Is the above OK? (maybe, it is ok in x64. but is it ok in C spec?).
I think we need a careful conversion.

> Source/JavaScriptCore/runtime/JSBigInt.h:187
> +    static JSBigInt* parseInt(ExecState* exec, VM& vm, StringView s)
> +    {
> +        if (s.is8Bit())
> +            return parseInt(exec, vm, s, s.characters8());
> +        return parseInt(exec, vm, s, s.characters16());
> +    }

Do we need this?

> Source/JavaScriptCore/runtime/JSBigInt.h:244
> +    template <typename CharType>
> +    static JSBigInt* parseInt(ExecState* exec, VM& vm, StringView s, CharType*  data)
> +    {
> +        int length = s.length();
> +
> +        int p = 0;
> +        while (p < length && isStrWhiteSpace(data[p]))
> +            ++p;
> +
> +        // Check Radix from frist characters
> +        if (p + 1 < length) {
> +            if (data[p] == '0' && (data[p + 1] == 'b' || data[p + 1] == 'B'))
> +                return parseInt(exec, vm, data, length, p + 2, 2, false);
> +
> +            if (data[p] == '0' && (data[p + 1] == 'x' || data[p + 1] == 'X'))
> +                return parseInt(exec, vm, data, length, p + 2, 16, false);
> +
> +            if (data[p] == '0' && (data[p + 1] == 'o' || data[p + 1] == 'O'))
> +                return parseInt(exec, vm, data, length, p + 2, 8, false);
> +        }
> +
> +        bool sign = false;
> +        if (p < length) {
> +            if (data[p] == '+')
> +                ++p;
> +            else if (data[p] == '-') {
> +                sign = true;
> +                ++p;
> +            }
> +        }
> +
> +        JSBigInt* result = parseInt(exec, vm, data, length, p, 10);
> +
> +        if (result && !result->isZero())
> +            result->setSign(sign);
> +
> +        return result;
> +    }

Ditto

> Source/JavaScriptCore/runtime/JSBigInt.h:300
> +    typedef uintptr_t Digit;

Let's use `using`.

> JSTests/stress/big-int-literals.js:105
> +assertThrowSyntaxError("1a0nn");

Let's add tests to ensure `10E20n` (including exponential part) is not allowed.

[Darin Adler]

Comment on attachment 327346 [details]
Patch

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

I’d like to see a lot more tests.

One example: doing "==" between a number and BigInt (to verify either that they always compare false, or that they compare based on the numeric value, whatever the rule is)

Another: Verify that we get an exception when we try to convert to a number in various ways.

Another: Verify the parsing of "+" and "-" and that you can’t have extra of either of those characters.

Another: Verify the behavior of various types of leading whitespace, making sure we strip the whitespace we are supposed to, and don’t strip whitespace we are not supposed to.

Another: Super-large numbers to test the out of memory errors.

There’s a lot of code added here not exercised by any test yet; we should do more of that.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1008
> +                return key.first->existingSymbolAwareHash() + IntHash<uint8_t>::hash(key.second);

Should just write:

    + WTF::intHash(key.second)

It’s overloaded for the unsigned integer types. No need to use the class template to get at that function.

>> Source/JavaScriptCore/parser/Lexer.cpp:1515
>> +    if (maximumDigits >= 0 && Options::useBigInt() && (m_current | 0x20) != 'n') {
> 
> This condition is wrong. If Options::useBigInt() is false, this condition never met.
> And could you add a test to cover the above thing?
> Ditto. And non-BigInt case should be handled as LIKELY in if-brace.

In new code, instead of expressions like (x | 0x20) != 'n', would you be willing to use the isASCIIAlphaCaselessEqual function? That’s what the function is designed for.

>> Source/JavaScriptCore/parser/Lexer.cpp:2217
>> +            parseResult = parseDecimal(tokenData->doubleValue);
> 
> `NumberParseResult parseResult = parseDecimal(tokenData->doubleValue);` is better.

I think we could even use auto there.

>> Source/JavaScriptCore/parser/Lexer.h:176
>> +    enum NumberParseResult {
> 
> Use `enum class`.

Also, for simple enumerations like this, it is nice to do it all on one line. The vertical style is needed for much longer ones.

> Source/JavaScriptCore/parser/Lexer.h:183
> +    ALWAYS_INLINE NumberParseResult parseHex(double& returnValue);

For new code, we are trying to not use out arguments, instead using structures or variants or expected or optional as appropriate. For these, I would suggest something like this:

    std::optional<Variant<double, Identifier>> parseDecimal();

Then if it’s not a number, it can return std::nullopt, if it’s a number it can return double, and if it’s a BigInt it can return it as an Identifier. Can be a little tricky to write the code if it’s your first time programming with Variant, but it usually comes out pretty good.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:89
> +#define HAVE_TWODIGIT 1

Seems overkill to use the HAVE macro for this thing that’s just in this one source file. I suggest using a straight #define. The configuration macros are really for project-wide things.

    #define HAVE_TWO_DIGIT 1
    #else
    #define HAVE_TWO_DIGIT 0
    #endif

Then below:

    #if HAVE_TWO_DIGIT

> Source/JavaScriptCore/runtime/JSBigInt.cpp:152
> +    Digit aLow  = a & HalfDigitMask;

WebKit specifically has a rule against "horizontally lined up" code like this. Instead we don’t bother trying to line up the "=" signs and let the roles be ragged.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:402
> +    if (exec)

What is this? Why can exec be null?

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

A duplicate of radixDigits from NumberPrototype.cpp but with a different name. Maybe we could share the array?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:409
> +String JSBigInt::toString(ExecState* exec, JSBigInt* x, int radix)

This function is really long. I suggest moving the charactersRequired computation into a separate function. Also, I don’t think that’s a great name for something that is likely to be an overestimate of the maximum number of characters required. I think maximumCharactersRequired is a better name.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:415
> +    if (x->isZero()) {
> +        StringBuilder resultBuilder;
> +        resultBuilder.append('0');
> +        return resultBuilder.toString().impl();
> +    }

Wrong to use StringBuilder to create constant single character string. Instead this would be the typical idiom:

    return ASCIILiteral { "0" };

But here in JavaScriptCore we have already have single character strings allocated, so we could write this, efficient because it doesn't allocate any memory:

    return exec->vm().smallStrings.singleCharacterStringRep('0');

> Source/JavaScriptCore/runtime/JSBigInt.cpp:420
> +    return toStringGeneric(exec, x, radix);
> +}
> +
> +String JSBigInt::toStringGeneric(ExecState* exec, JSBigInt* x, int radix)

Unclear why these are two separate functions.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:422
> +    StringBuilder resultBuilder;

Wrong to use a StringBuilder for this, since we don’t build a string with it, and we don’t need the features of StringBuilder, which is good at building strings from complex combinations of strings, characters, literals, and numbers. We should use StringVector<LChar>.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:427
> +    const int length = x->length();

I don’t understand the use of const in local variables here. If we are using it here, then...

> Source/JavaScriptCore/runtime/JSBigInt.cpp:433
> +    int leadingZeros = clz64(x->digit(length - 1));

Why not here?

I suggest just not using it for local variables. Or using it more consistently perhaps?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:462
> +    int pos = 0;

Using a StringVector we won’t need this "pos" because it's a duplicate copy of the size of the StringVector.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:465
> +    resultBuilder.reserveCapacity(static_cast<size_t>(charsRequired));

For a StringVector we don’t necessarily need to do reserveInitialCapacity, but we could. If we do, then we could also use uncheckedAppend.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:520
> +    while (pos > 1 && resultBuilder[pos - 1] == '0')
> +        pos--;

Using StringVector this would be done by calling shrink; ideally just one time, but could also call it in a loop.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:531
> +    StringBuilder reverseBuilder;
> +    reverseBuilder.reserveCapacity(static_cast<size_t>(charsRequired));
> +    for (int i = pos - 1; i >= 0; i--)
> +        reverseBuilder.append(resultBuilder[i]);

This is another reason to not use a StringBuilder; we don’t even use the initial one to build a string, just to keep characters in it! And then in our second StringBuilder we know exactly how many characters we need, but for some reason we reserve charsRequired instead of pos. And since we know exactly how many characters we need we would have used StringImp::createUninitialized instead of StringBuilder.

But if we use StringVector from the start, then we don’t need to allocate a second copy of all the characters. We can reverse the characters in place:

    std::reverse(vector.begin(), vector.end());

> Source/JavaScriptCore/runtime/JSBigInt.cpp:533
> +    return reverseBuilder.toString().impl();

Here we can use StringImpl::adopt(vector);

> Source/JavaScriptCore/runtime/JSBigInt.cpp:545
> +    if (this->isZero())

No need for "this->" here.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:560
> +        data[i] = this->digit(i);

No need for "this->" here.

> Source/JavaScriptCore/runtime/JSBigInt.h:43
> +

Stray blank line here.

> Source/JavaScriptCore/runtime/JSBigInt.h:52
> +    inline static int sizeFor(int length)

The inline keyword here is not needed; ignored by the compiler.

I think this function should be private; not good to call outside the class, and not needed.

> Source/JavaScriptCore/runtime/JSBigInt.h:62
> +    static JSBigInt* createZero(ExecState* exec, VM& vm)

I think that classes are easier to read if they don’t combine their implementation and interface. So the inline functions would be separate from the class definition. In the class definition we would just have the declarations of these functions. Then you could more easily scan and see wha the interface of JSBigInt is.

It also makes it a lot easier to move functions out of the header if we decide they should not be inline; just take out the inline keyword and move into the .cpp file. Many of these functions are really long and I am not sure we should try to inline them.

> Source/JavaScriptCore/runtime/JSBigInt.h:69
> +    static JSBigInt* create(ExecState* exec, VM& vm, int length)

Quite unclear that the third argument is the length; so easy to confuse this with createFromInt. Should name this createWithLength.

> Source/JavaScriptCore/runtime/JSBigInt.h:76
> +    static JSBigInt* createFromInt(ExecState* exec, VM& vm, int32_t value)

If someone tries to pass an int64_t to this function, it will compile and just silently chop off the high bits. It would be safer to overload this one with other functions that create from various types because then if someone tries to pass an integer of another type will get a compiler error because it’s ambiguous. That’s why functions like StringBuilder::appendNumber do not have the integer type in their names and use overloading instead.

> Source/JavaScriptCore/runtime/JSBigInt.h:93
> +    static JSBigInt* createFromUint(ExecState* exec, VM& vm, uint32_t value)

Please overload (see reasoning above).

> Source/JavaScriptCore/runtime/JSBigInt.h:104
> +    static JSBigInt* createFromAnyInt(ExecState* exec, VM& vm, int64_t value)

Please overload (see reasoning above).

>> Source/JavaScriptCore/runtime/JSBigInt.h:114
>> +            bigInt->setDigit(0, static_cast<Digit>(-1 * value));
> 
> Consider INT64_MIN case. -INT64_MIN is not representable in int64_t. Is the above OK? (maybe, it is ok in x64. but is it ok in C spec?).
> I think we need a careful conversion.

It’s also unnecessary to write -1 * value instead of just -value.

> Source/JavaScriptCore/runtime/JSBigInt.h:139
> +    static JSBigInt* createFromBoolean(ExecState* exec, VM& vm, bool value)

Consider overloading instead of a separate function.

> Source/JavaScriptCore/runtime/JSBigInt.h:180
> +    static String toString(ExecState*, JSBigInt*, int radix);

Why not a non-static member function instead of a static member function that takes a JSBigInt*?

> Source/JavaScriptCore/runtime/JSBigInt.h:208
> +    static JSBigInt* parseInt(ExecState* exec, VM& vm, StringView s, CharType*  data)

Two spaces after the "*" here.

> Source/JavaScriptCore/runtime/JSBigInt.h:216
> +        // Check Radix from frist characters

Typo "frist".

> Source/JavaScriptCore/runtime/JSBigInt.h:218
> +            if (data[p] == '0' && (data[p + 1] == 'b' || data[p + 1] == 'B'))

These should use isASCIIAlphaCaselessEqual; more efficient than the ||, I think.

> Source/JavaScriptCore/runtime/JSBigInt.h:309
> +

Stray blank line here.

> Source/JavaScriptCore/runtime/JSBigInt.h:317
> +

Stray blank line here.

> Source/JavaScriptCore/runtime/JSBigInt.h:324
> +    static inline Digit digitAdd(Digit a, Digit b, Digit* carry);
> +    static inline Digit digitSub(Digit a, Digit b, Digit* borrow);
> +    static inline Digit digitMul(Digit a, Digit b, Digit* high);
> +    static inline Digit digitDiv(Digit high, Digit low, Digit divisor, Digit* remainder);

These seem like they should be references, not pointers.

The "inline" here aren’t needed. The inline in the function definitions are sufficient.

But also, why are these member functions at all? They can just be non-member functions in the .cpp file, I think. No reason to have to touch the header when changing these around. Same for most of the private static member functions; only ones used in the header need to be static member functions.

> Source/JavaScriptCore/runtime/JSBigInt.h:327
> +    static void absoluteDivSmall(ExecState*, JSBigInt* x, Digit divisor, JSBigInt** quotient, Digit* remainder);

Ditto, except maybe JSBigInt because I heard JavaScriptCore programmers saying they never want to use references for those.

> Source/JavaScriptCore/runtime/JSBigInt.h:332
> +    inline Digit digit(int n)

The inline here is not needed; ignored by the compiler.

> Source/JavaScriptCore/runtime/JSBigInt.h:340
> +    inline void setDigit(int n, Digit value)

The inline here is not needed; ignored by the compiler.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:384
> +        String bigIntString = JSBigInt::toString(exec, asBigInt(asCell()), 10);
> +        if (bigIntString.length() == 1)
> +            return vm.smallStrings.singleCharacterString(bigIntString[0]);
> +        return jsNontrivialString(&vm, bigIntString);

This is missing a WTFMove in the calls to jsNontrivialString.

Should use pass *this to asBigInt instead of asCell().

A little sad to create a temporary string if this really is a JSBigInt in the range 0-9 and we want that to be a fast path. More efficient to add a JSBigInt function that can give us the integer values for small BigInt so we can check that it’s in the range 0-9. Or if we don't need to optimize the code path so much, go the other way and just use jsString instead of writing out the single character case explicitly:

    return jsString(&vm, JSBigInt::toString(exec, asBigInt(asCell()), 10));

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:730
> +    if (isBigInt()) {

We don’t need this type error behavior to be implemented as part of the fast case, slightly slowing it down; please move this logic into toNumberSlowCase.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:731
> +        throwTypeError(exec, scope, ASCIILiteral("Cast from 'BigInt' to 'number' is not allowed."));

I don’t think the word "cast" here makes sense to JavaScript programmers. It’s not a JavaScript term.

[Caio Lima]

Comment on attachment 327346 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1004
>> +        typedef std::pair<UniquedStringImpl*, uint8_t> BigIntMapEntry;
> 
> Use `using` instead of `typedef`

Done.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1008
>> +                return key.first->existingSymbolAwareHash() + IntHash<uint8_t>::hash(key.second);
> 
> Should just write:
> 
>     + WTF::intHash(key.second)
> 
> It’s overloaded for the unsigned integer types. No need to use the class template to get at that function.

Done. Thanks.

>>> Source/JavaScriptCore/parser/Lexer.cpp:1515
>>> +    if (maximumDigits >= 0 && Options::useBigInt() && (m_current | 0x20) != 'n') {
>> 
>> This condition is wrong. If Options::useBigInt() is false, this condition never met.
>> And could you add a test to cover the above thing?
>> Ditto. And non-BigInt case should be handled as LIKELY in if-brace.
> 
> In new code, instead of expressions like (x | 0x20) != 'n', would you be willing to use the isASCIIAlphaCaselessEqual function? That’s what the function is designed for.

@yusuke You are right.

@darin, I think I have wrong logic here. BigInt literals doesn't allow "N" as suffix.

>> Source/JavaScriptCore/parser/Lexer.cpp:1537
>> +        return BigInt;
> 
> Can we use BigInt without Options::useBigInt() check?

No. Fixed. Thanks for catching it.

>> Source/JavaScriptCore/parser/Lexer.cpp:2209
>> +            if (parseOctal(tokenData->doubleValue) == NumberParseResult::Number) {
> 
> What happens if it returns BigInt?

I think it's not very easy to follow and probably we should change this logic. It will follow the same as if we have 012e10, for example. It will FALLTHROUGH to number case, then m_buffer will be filled with consumed digits, so, parseDecimal will encounter "n" char and will return BigInt, and finally the number will be created. I'm adding a test case for it.

>>> Source/JavaScriptCore/parser/Lexer.cpp:2217
>>> +            parseResult = parseDecimal(tokenData->doubleValue);
>> 
>> `NumberParseResult parseResult = parseDecimal(tokenData->doubleValue);` is better.
> 
> I think we could even use auto there.

@Yusuke, I can't do that because of "inNumberAfterDecimalPoint:" into line 2228. It is resulting into following compilation error:

```./parser/Lexer.cpp:2116:9: error: cannot jump from this goto statement to its label
        goto inNumberAfterDecimalPoint;
        ^
./parser/Lexer.cpp:2216:31: note: jump bypasses variable initialization
            NumberParseResult parseResult = parseDecimal(tokenData->doubleValue);
```
Not sure how could I remove goto and avoid repeat code on line 2116. Maybe replicate code up there is better for us.

@Darin: I don't think I like use auto here because it makes code harder to follow.

>> Source/JavaScriptCore/parser/NodeConstructors.h:96
>> +        : ConstantNode(location, ResultType::unknownType())
> 
> Let's add ResultType::bigIntType() and its handling in ResultType.h.

Ok.

>> Source/JavaScriptCore/parser/Parser.cpp:4496
>> +        const Identifier* ident = m_token.m_data.ident;
> 
> Retrieve m_token.m_data.bigIntString.

ok.

>> Source/JavaScriptCore/parser/ParserTokens.h:221
>>      };
> 
> We should add a new struct definition to this union for BigInt for readability.
> Like,
> 
> struct {
>     const Identifier* bigIntString;
>     uint32_t radix;
> };

ok.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:402
>> +    if (exec)
> 
> What is this? Why can exec be null?

exec is null when we create JSBigInt from BytecodeGenerator::addBigIntConstant. If I understand correctly, there is no exec but that time. Is it right?

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:407
>> +static const char ConversionChars[] = "0123456789abcdefghijklmnopqrstuvwxyz";
> 
> A duplicate of radixDigits from NumberPrototype.cpp but with a different name. Maybe we could share the array?

I agree. Do you have any idea where I cloud place this array?

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:420
>> +String JSBigInt::toStringGeneric(ExecState* exec, JSBigInt* x, int radix)
> 
> Unclear why these are two separate functions.

I'm planing to create a special case when radix is base of 2, just like V8 implementation.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:531
>> +        reverseBuilder.append(resultBuilder[i]);
> 
> This is another reason to not use a StringBuilder; we don’t even use the initial one to build a string, just to keep characters in it! And then in our second StringBuilder we know exactly how many characters we need, but for some reason we reserve charsRequired instead of pos. And since we know exactly how many characters we need we would have used StringImp::createUninitialized instead of StringBuilder.
> 
> But if we use StringVector from the start, then we don’t need to allocate a second copy of all the characters. We can reverse the characters in place:
> 
>     std::reverse(vector.begin(), vector.end());

I like it. Going to change it. And you are right, I'm using reserveCapacity with wrong parameter here.

>> Source/JavaScriptCore/runtime/JSBigInt.h:62
>> +    static JSBigInt* createZero(ExecState* exec, VM& vm)
> 
> I think that classes are easier to read if they don’t combine their implementation and interface. So the inline functions would be separate from the class definition. In the class definition we would just have the declarations of these functions. Then you could more easily scan and see wha the interface of JSBigInt is.
> 
> It also makes it a lot easier to move functions out of the header if we decide they should not be inline; just take out the inline keyword and move into the .cpp file. Many of these functions are really long and I am not sure we should try to inline them.

I agree. I'm going to refactor the code following it.

>> Source/JavaScriptCore/runtime/JSBigInt.h:69
>> +    static JSBigInt* create(ExecState* exec, VM& vm, int length)
> 
> Quite unclear that the third argument is the length; so easy to confuse this with createFromInt. Should name this createWithLength.

ok.

>> Source/JavaScriptCore/runtime/JSBigInt.h:76
>> +    static JSBigInt* createFromInt(ExecState* exec, VM& vm, int32_t value)
> 
> If someone tries to pass an int64_t to this function, it will compile and just silently chop off the high bits. It would be safer to overload this one with other functions that create from various types because then if someone tries to pass an integer of another type will get a compiler error because it’s ambiguous. That’s why functions like StringBuilder::appendNumber do not have the integer type in their names and use overloading instead.

This is good information.

>> Source/JavaScriptCore/runtime/JSBigInt.h:85
>> +            bigInt->setDigit(0, static_cast<Digit>(-1 * value));
> 
> Consider INT32_MIN case. -INT32_MIN is not representable in int32_t. I think we should extend value to 64bit first and negate it.

You are right, nice catch.

>>> Source/JavaScriptCore/runtime/JSBigInt.h:114
>>> +            bigInt->setDigit(0, static_cast<Digit>(-1 * value));
>> 
>> Consider INT64_MIN case. -INT64_MIN is not representable in int64_t. Is the above OK? (maybe, it is ok in x64. but is it ok in C spec?).
>> I think we need a careful conversion.
> 
> It’s also unnecessary to write -1 * value instead of just -value.

You are right. This code is meant to use when int53 is the case and they are going to be used into BigIntConstructor Patch. I think It is better remove them from this Patch and implement them there.

>> Source/JavaScriptCore/runtime/JSBigInt.h:139
>> +    static JSBigInt* createFromBoolean(ExecState* exec, VM& vm, bool value)
> 
> Consider overloading instead of a separate function.

ok. BTW, I'm handling it in https://bugs.webkit.org/show_bug.cgi?id=175359, since it's not being used in this Patch.

>> Source/JavaScriptCore/runtime/JSBigInt.h:187
>> +    }
> 
> Do we need this?

Not in this Patch. removing them and adding into https://bugs.webkit.org/show_bug.cgi?id=175359

>> Source/JavaScriptCore/runtime/JSBigInt.h:218
>> +            if (data[p] == '0' && (data[p + 1] == 'b' || data[p + 1] == 'B'))
> 
> These should use isASCIIAlphaCaselessEqual; more efficient than the ||, I think.

ok.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:730
>> +    if (isBigInt()) {
> 
> We don’t need this type error behavior to be implemented as part of the fast case, slightly slowing it down; please move this logic into toNumberSlowCase.

ok.

>> JSTests/stress/big-int-literals.js:105
>> +assertThrowSyntaxError("1a0nn");
> 
> Let's add tests to ensure `10E20n` (including exponential part) is not allowed.

sure. Thanks.

[Caio Lima]

(In reply to Darin Adler from comment #16)
> Comment on attachment 327346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327346&action=review
> 
> I’d like to see a lot more tests.
> 
> One example: doing "==" between a number and BigInt (to verify either that
> they always compare false, or that they compare based on the numeric value,
> whatever the rule is)

This require the "==" operator to be implemented. It's going to be handled into separate Bug (https://bugs.webkit.org/show_bug.cgi?id=179901).

> Another: Verify that we get an exception when we try to convert to a number
> in various ways.

I think my last comment follow. Here we need to implement other operations as well. the scope of this Bug is to enable literal parsing and introduce JSBigInt object. 

> Another: Verify the parsing of "+" and "-" and that you can’t have extra of
> either of those characters.

Do you mean "+" and "-" unary operations? They also are going to be handled into https://bugs.webkit.org/show_bug.cgi?id=179900

> Another: Verify the behavior of various types of leading whitespace, making
> sure we strip the whitespace we are supposed to, and don’t strip whitespace
> we are not supposed to.

ok.

> Another: Super-large numbers to test the out of memory errors.
 ok.

[Darin Adler]

(In reply to Caio Lima from comment #18)
> (In reply to Darin Adler from comment #16)
> > Another: Verify the parsing of "+" and "-" and that you can’t have extra of
> > either of those characters.
> 
> Do you mean "+" and "-" unary operations? They also are going to be handled
> into https://bugs.webkit.org/show_bug.cgi?id=179900

No, I mean "+" and "-" in BigInt parsing, like "+123123123123123123123" and "-123123123123123123123", and invalid things like "--123123123123123123123".

[Caio Lima]

(In reply to Darin Adler from comment #19)
> (In reply to Caio Lima from comment #18)
> > (In reply to Darin Adler from comment #16)
> > > Another: Verify the parsing of "+" and "-" and that you can’t have extra of
> > > either of those characters.
> > 
> > Do you mean "+" and "-" unary operations? They also are going to be handled
> > into https://bugs.webkit.org/show_bug.cgi?id=179900
> 
> No, I mean "+" and "-" in BigInt parsing, like "+123123123123123123123" and
> "-123123123123123123123", and invalid things like "--123123123123123123123".

just to make clear you mean:

```
let a = -123123123123123123123n;
a = +123123123123123123123n;
```

In such cases, they are unary operators that are handled as fast path on AST construction. For example, "ASTBuilder::makeNegateNode" creates "-3" as constant, but Lexer creates MINUS token and INTEGER token as well. I'm planing add such fast path into Unary operations Patch, but if you require it, I could add it here. However, IMHO this Patch is too large to keep adding such things. About "--123123123123123123123" I agree with you and I'll add tests with them.

[Darin Adler]

Comment on attachment 327346 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSBigInt.h:235
> +            if (data[p] == '+')
> +                ++p;
> +            else if (data[p] == '-') {
> +                sign = true;
> +                ++p;
> +            }

In my comments about tests I’m referring to test coverage for whatever this code is doing.

[Caio Lima]

(In reply to Darin Adler from comment #21)
> Comment on attachment 327346 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327346&action=review
> 
> > Source/JavaScriptCore/runtime/JSBigInt.h:235
> > +            if (data[p] == '+')
> > +                ++p;
> > +            else if (data[p] == '-') {
> > +                sign = true;
> > +                ++p;
> > +            }
> 
> In my comments about tests I’m referring to test coverage for whatever this
> code is doing.

Oh, I see. Actually this code is going to be used in BigIntCosntructor like ``` let a = BigInt("-12123");``` for example. I was firstly working into this Bug https://bugs.webkit.org/show_bug.cgi?id=175359 and when Patch became huge, I decided split the JSBigInt implementation with this Patch. I'm removing this code form this Patch and add it into upcoming https://bugs.webkit.org/show_bug.cgi?id=175359.

[Caio Lima]

Comment on attachment 327346 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSBigInt.h:180
>> +    static String toString(ExecState*, JSBigInt*, int radix);
> 
> Why not a non-static member function instead of a static member function that takes a JSBigInt*?

Changed.

>> Source/JavaScriptCore/runtime/JSBigInt.h:324
>> +    static inline Digit digitDiv(Digit high, Digit low, Digit divisor, Digit* remainder);
> 
> These seem like they should be references, not pointers.
> 
> The "inline" here aren’t needed. The inline in the function definitions are sufficient.
> 
> But also, why are these member functions at all? They can just be non-member functions in the .cpp file, I think. No reason to have to touch the header when changing these around. Same for most of the private static member functions; only ones used in the header need to be static member functions.

They are members because of Digit type is private to JSBigInt. I don't think it is necessary expose this type publicly.

>> Source/JavaScriptCore/runtime/JSCJSValue.cpp:384
>> +        return jsNontrivialString(&vm, bigIntString);
> 
> This is missing a WTFMove in the calls to jsNontrivialString.
> 
> Should use pass *this to asBigInt instead of asCell().
> 
> A little sad to create a temporary string if this really is a JSBigInt in the range 0-9 and we want that to be a fast path. More efficient to add a JSBigInt function that can give us the integer values for small BigInt so we can check that it’s in the range 0-9. Or if we don't need to optimize the code path so much, go the other way and just use jsString instead of writing out the single character case explicitly:
> 
>     return jsString(&vm, JSBigInt::toString(exec, asBigInt(asCell()), 10));

I agree. Fixed.

[Caio Lima]

Created attachment 327481 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:160:  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:444:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:445:  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:447:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/parser/Lexer.cpp:2223:  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 48 files


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

[Caio Lima]

Created attachment 327501 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:161:  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:445:  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:447:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:448:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/parser/Lexer.cpp:2223:  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 50 files


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

[Darin Adler]

Comment on attachment 327501 [details]
Patch

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

Did not review the whole thing, but did a quick look and spotted some issues.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3145
> +        bigIntInMap = JSBigInt::parseInt(nullptr, *vm(), StringView(identifier.string()), radix);

Should not have to write StringView() explicitly here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3148
> +            m_error = ParserError(ParserError::OutOfMemory);
> +            return jsUndefined();

This will leave a nullptr behind in the map. Subsequent calls will get the nullptr value and no parser error. Is that really want we want? Need a test case that covers this, with the same invalid constant twice. Except that this says "out of memory"; I’m not sure I understand what this case is for exactly

> Source/JavaScriptCore/parser/Lexer.cpp:1599
> +    if (LIKELY(!isASCIIDigit(m_current) && digit >= 0 && m_current != 'n')) {
> +        return std::optional<Variant<double, const Identifier*>>(octalValue);
>      }

WebKit coding style says no braces around a single line if statement body like this.

> Source/JavaScriptCore/parser/Lexer.cpp:2105
> +        if ((m_current | 0x20) == 'e') {

if (isASCIIAlphaCaselessEqual(m_current, 'e')) {

> Source/JavaScriptCore/parser/Lexer.cpp:2137
> +            if (parseNumberResult.index() == 1) {

I think it’s more elegant to write this:

    if (WTF::holds_alternative<const Identifier*>(parseNumberResult)) {

Then there’s no magic number 1. But also, I would put the shorter "double" case first. I find if statements easier to read with the shorter case first.

> Source/JavaScriptCore/parser/Lexer.cpp:2166
> +            if (parseNumberResult && parseNumberResult->index() == 1) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2196
> +            if (numberParseResult && numberParseResult->index() == 1) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2223
> +            if (numberParseResult && numberParseResult->index() == 0) {

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:2232
> +            if (!parseNumberResult || parseNumberResult->index() == 1) {

Ditto.

> Source/JavaScriptCore/parser/ResultType.h:40
> +        static const Type TypeMaybeBigInt = 0x16;

I don’t understand this. All the other type flags are powers of two so they are single bits that don’t overlap other bits. But this combination of bits overlaps bool, string, and number. Why is that OK? I don’t think it is.

> Source/JavaScriptCore/parser/ResultType.h:89
> +        bool mightBeBigInt() const

Can we leave these out for now? I see no uses of them. I know that number has similar functions, but I don’t think we will need these. Or do you know that we will need these and how we will use them?

> Source/JavaScriptCore/parser/ResultType.h:94
> +        bool isNotBigInt() const

Ditto.

[Caio Lima]

Comment on attachment 327501 [details]
Patch

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

Thanks for the review!

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3148
>> +            return jsUndefined();
> 
> This will leave a nullptr behind in the map. Subsequent calls will get the nullptr value and no parser error. Is that really want we want? Need a test case that covers this, with the same invalid constant twice. Except that this says "out of memory"; I’m not sure I understand what this case is for exactly

You are right. I don't want to have null entries here. This case here happens when BigInt parsing throws OOM. The test case covering it is JSTests/stress/big-int-large-number-oom.js. Basically you have a literal that can't be parsed because it is large than our allowed size.

>> Source/JavaScriptCore/parser/Lexer.cpp:2137
>> +            if (parseNumberResult.index() == 1) {
> 
> I think it’s more elegant to write this:
> 
>     if (WTF::holds_alternative<const Identifier*>(parseNumberResult)) {
> 
> Then there’s no magic number 1. But also, I would put the shorter "double" case first. I find if statements easier to read with the shorter case first.

That's far better. Thanks!

>> Source/JavaScriptCore/parser/ResultType.h:40
>> +        static const Type TypeMaybeBigInt = 0x16;
> 
> I don’t understand this. All the other type flags are powers of two so they are single bits that don’t overlap other bits. But this combination of bits overlaps bool, string, and number. Why is that OK? I don’t think it is.

Oh. that's my bad. it is in hex and I was reading 0x10 as 10, not 16...Nice catch.

>> Source/JavaScriptCore/parser/ResultType.h:89
>> +        bool mightBeBigInt() const
> 
> Can we leave these out for now? I see no uses of them. I know that number has similar functions, but I don’t think we will need these. Or do you know that we will need these and how we will use them?

We will use then in the further Patches to handle BytecodeGenerator optimizations such as other types. I was thinking in add them there, but @yusuke asked me to add them in this Patch. In his last review.

[Caio Lima]

Created attachment 327519 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:161:  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:445:  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:447:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:448:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 51 files


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

[Caio Lima]

Created attachment 327568 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:161:  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:445:  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:447:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:448:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 53 files


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

[Yusuke Suzuki]

Comment on attachment 327568 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:99
> +    m_error = ParserError(ParserError::ErrorNone);

Drop this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:219
> +    if (m_error.isValid())
> +        return m_error;
> +

Drop this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3152
> +    if (!bigIntInMap) {
> +        m_error = ParserError(ParserError::OutOfMemory);
> +        return jsUndefined();
> +    }

I think currently, we have no way to handle this correctly.
Just RELEASE_ASSERT would be better since we do not have the same handling for JSString.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1231
> +        ParserError m_error;

Drop this.

> Source/JavaScriptCore/jsc.cpp:48
> +#include "JSBigInt.h"

Why is this necessary?

> Source/JavaScriptCore/llint/LLIntData.cpp:170
> +        uint32_t bits = 0x480000;

Why do we need this change?

> Source/JavaScriptCore/llint/LLIntData.cpp:180
> +        uint32_t bits = 0x880000;

Ditto.

> Source/JavaScriptCore/llint/LLIntData.cpp:190
> +        uint32_t bits = 0x900000;

Ditto.

> Source/JavaScriptCore/llint/LLIntData.cpp:200
> +        uint32_t bits = 0x500000;

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1640
> +        if (digit >= 0 && m_current != '.' && (m_current | 0x20) != 'e' && m_current != 'n')

Use isASCIIAlphaCaselessEqual.

> Source/JavaScriptCore/parser/NodeConstructors.h:96
> +        : ConstantNode(location, ResultType::unknownType())

ResultType::bigIntType().

> Source/JavaScriptCore/parser/ResultType.h:146
> +            if (op1.definitelyIsBigInt() || op2.definitelyIsBigInt())
> +                return op1.definitelyIsBigInt() && op2.definitelyIsBigInt() ? bigIntType() : unknownType();

We should model this precisely according to this https://tc39.github.io/proposal-bigint/#sec-addition-operator-plus-runtime-semantics-evaluation. Otherwise, it becomes a bug.
Right now, we fallback to stringOrNumberType if we do not have any information about op1 and op2. But it is wrong since it does not include bigIntType().

Instead of returning unknownType() / stringOrNumberType(), we should fallback to something like `addResultType()` instead of `stringOrNumberType()`.
And `addResultType()` should include String, Number, and BigInt.

So, here, we should have

if (op1.definitelyIsBigInt() || op2.definitelyIsBigInt())
    return bigIntType();
return addResultType();

Because one of op is definitely BigInt, the result should be BigInt or TypeError.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:75
> +JSBigInt::JSBigInt(VM& vm, Structure* structure, int createWithLength)
> +    : Base(vm, structure)
> +    , m_length(createWithLength)

No, I think darin@ said we should rename `JSBigInt::create` to `JSBigInt::createWithLength`.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:101
> +JSBigInt* JSBigInt::create(ExecState* exec, VM& vm, int createWithLength)

createWithLength.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:121
> +        void* backingStore = vm.gigacageAuxiliarySpace(m_data.kind).tryAllocate(sizeFor(m_length));
> +        Digit* data = static_cast<Digit*>(backingStore);
> +        if (!data) {
> +            auto scope = DECLARE_THROW_SCOPE(vm);
> +            throwOutOfMemoryError(exec, scope);
> +            return;
> +        }
> +        
> +        m_data.set(vm, this, data);

It's ok for now. But I'm not sure why we need to allocate auxiliary buffer for BigInt. Why not allocating `sizeof(JSBigInt) + alignment + sizeof(Digit) * length` for JSBigInt and using tail space as its buffer? (this is similar design to JSLexicalEnvironment).

> Source/JavaScriptCore/runtime/JSBigInt.h:41
> +    friend class BigIntImpl;

Let's drop this.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:73
> +    VM& vm = exec->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

This is unnecessary.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:82
> +    if (isBigInt()) {
> +        throwTypeError(exec, scope, ASCIILiteral("Convertion from 'BigInt' to 'number' is not allowed."));
> +        return PNaN;
> +    }

This is wrong. JSBigInt case should be handled in `if (isCell())` clause.
We should add this to `JSCell::toNumber`.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:728
> +

Drop this.

> Source/JavaScriptCore/runtime/JSCell.cpp:161
>  }

Please revisit all the JSCell's functions. Bunch of functions just assume that Symbol, JSString, and JSObject may come. But JSBigInt is not considered.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:640
> -    
> +

Let's drop this.

> Source/JavaScriptCore/runtime/PrototypeKey.h:32
> +class FunctionExecutable;

Why is this necessary?

> Source/JavaScriptCore/runtime/StructureCache.h:36
> +class FunctionExecutable;

Ditto.

[Yusuke Suzuki]

Comment on attachment 327568 [details]
Patch

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

>> Source/JavaScriptCore/parser/ResultType.h:146
>> +                return op1.definitelyIsBigInt() && op2.definitelyIsBigInt() ? bigIntType() : unknownType();
> 
> We should model this precisely according to this https://tc39.github.io/proposal-bigint/#sec-addition-operator-plus-runtime-semantics-evaluation. Otherwise, it becomes a bug.
> Right now, we fallback to stringOrNumberType if we do not have any information about op1 and op2. But it is wrong since it does not include bigIntType().
> 
> Instead of returning unknownType() / stringOrNumberType(), we should fallback to something like `addResultType()` instead of `stringOrNumberType()`.
> And `addResultType()` should include String, Number, and BigInt.
> 
> So, here, we should have
> 
> if (op1.definitelyIsBigInt() || op2.definitelyIsBigInt())
>     return bigIntType();
> return addResultType();
> 
> Because one of op is definitely BigInt, the result should be BigInt or TypeError.

Ah, no. If one of op may be string, the result would be string. So, this should be,

if (op1.definitelyIsBigInt() && op2.definitelyIsBigInt())
    return bigIntType();
return addResultType();

[Caio Lima]

Comment on attachment 327568 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3152
>> +    }
> 
> I think currently, we have no way to handle this correctly.
> Just RELEASE_ASSERT would be better since we do not have the same handling for JSString.

Ok. Created the bug. https://bugs.webkit.org/show_bug.cgi?id=180139

>> Source/JavaScriptCore/jsc.cpp:48
>> +#include "JSBigInt.h"
> 
> Why is this necessary?

No.

>> Source/JavaScriptCore/llint/LLIntData.cpp:170
>> +        uint32_t bits = 0x480000;
> 
> Why do we need this change?

We are changing ResultType::numBitsNeeded to 7. ArithProfile uses it to calculate rhsObservedTypeShift and lhsObservedTypeShift. So, we need to << 2 all hard coded bits here.

>>> Source/JavaScriptCore/parser/ResultType.h:146
>>> +                return op1.definitelyIsBigInt() && op2.definitelyIsBigInt() ? bigIntType() : unknownType();
>> 
>> We should model this precisely according to this https://tc39.github.io/proposal-bigint/#sec-addition-operator-plus-runtime-semantics-evaluation. Otherwise, it becomes a bug.
>> Right now, we fallback to stringOrNumberType if we do not have any information about op1 and op2. But it is wrong since it does not include bigIntType().
>> 
>> Instead of returning unknownType() / stringOrNumberType(), we should fallback to something like `addResultType()` instead of `stringOrNumberType()`.
>> And `addResultType()` should include String, Number, and BigInt.
>> 
>> So, here, we should have
>> 
>> if (op1.definitelyIsBigInt() || op2.definitelyIsBigInt())
>>     return bigIntType();
>> return addResultType();
>> 
>> Because one of op is definitely BigInt, the result should be BigInt or TypeError.
> 
> Ah, no. If one of op may be string, the result would be string. So, this should be,
> 
> if (op1.definitelyIsBigInt() && op2.definitelyIsBigInt())
>     return bigIntType();
> return addResultType();

Sure. Nice Catch.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:75
>> +    , m_length(createWithLength)
> 
> No, I think darin@ said we should rename `JSBigInt::create` to `JSBigInt::createWithLength`.

Ooh. Makes sense and it is better as well.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:121
>> +        m_data.set(vm, this, data);
> 
> It's ok for now. But I'm not sure why we need to allocate auxiliary buffer for BigInt. Why not allocating `sizeof(JSBigInt) + alignment + sizeof(Digit) * length` for JSBigInt and using tail space as its buffer? (this is similar design to JSLexicalEnvironment).

I agree. I just didn't know that I could put int "inline" but I noticed that JSFinalObject does it as well with inlineStorage. I'm going to follow it.

>> Source/JavaScriptCore/runtime/PrototypeKey.h:32
>> +class FunctionExecutable;
> 
> Why is this necessary?

It was breaking build in my old Patch, because this class isn't forward declared into any Header. Let me drop it and see if it breaks current Patch.

[Caio Lima]

Created attachment 327970 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:150:  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:434:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:435:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:436:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:437:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 49 files


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

[Yusuke Suzuki]

Comment on attachment 327970 [details]
Patch

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

Patch looks quite nice.

> Source/JavaScriptCore/llint/LLIntData.cpp:170
> +        uint32_t bits = 0x480000;

Need to sync LowLevelInterpreter.asm's data.

> Source/JavaScriptCore/llint/LLIntData.cpp:180
> +        uint32_t bits = 0x880000;

Need to sync LowLevelInterpreter.asm's data.

> Source/JavaScriptCore/llint/LLIntData.cpp:190
> +        uint32_t bits = 0x900000;

Need to sync LowLevelInterpreter.asm's data.

> Source/JavaScriptCore/llint/LLIntData.cpp:200
> +        uint32_t bits = 0x500000;

Need to sync LowLevelInterpreter.asm's data.

> Source/JavaScriptCore/parser/Lexer.cpp:1560
> +        return std::optional<Variant<double, const Identifier*>>(binaryValue);

I think `return binaryValue;` just works.

> Source/JavaScriptCore/parser/Lexer.cpp:1571
> +        return std::optional<Variant<double, const Identifier*>>(makeIdentifier(m_buffer8.data(), m_buffer8.size()));

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1576
> +    return std::optional<Variant<double, const Identifier*>>(parseIntOverflow(m_buffer8.data(), m_buffer8.size(), 2));

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1598
> +        return std::optional<Variant<double, const Identifier*>>(octalValue);

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1610
> +        return std::optional<Variant<double, const Identifier*>>(makeIdentifier(m_buffer8.data(), m_buffer8.size()));

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1615
> +    return std::optional<Variant<double, const Identifier*>>(parseIntOverflow(m_buffer8.data(), m_buffer8.size(), 8));

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1641
> +            return std::optional<Variant<double, const Identifier*>>(decimalValue);

Ditto.

> Source/JavaScriptCore/parser/Lexer.cpp:1653
> +        return std::optional<Variant<double, const Identifier*>>(makeIdentifier(m_buffer8.data(), m_buffer8.size()));

Ditto.

> Source/JavaScriptCore/parser/Lexer.h:176
> +    enum class NumberParseResult { Number, BigInt, Failure };

It is no longer used. Let's drop this.

> Source/JavaScriptCore/runtime/JSBigInt.h:44
> +    void initialize(bool zeroInitialize);

Pass some `enum class` instead of `bool`.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:77
> +

This line is unnecessary.

> Source/JavaScriptCore/runtime/JSCell.cpp:190
> +    // FIXME: [ESNext][BigInt] Implement JSBigInt, BigIntConstructor and BigIntPrototyp

Typo, /BigIntPrototyp/BigIntPrototype/

> Source/JavaScriptCore/runtime/Structure.cpp:29
> +#include "BuiltinNames.h"

Is this necessary?

[Caio Lima]

Comment on attachment 327970 [details]
Patch

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

Thanks for the review!

>> Source/JavaScriptCore/parser/Lexer.cpp:1560
>> +        return std::optional<Variant<double, const Identifier*>>(binaryValue);
> 
> I think `return binaryValue;` just works.

It is generating compilation error.

>> Source/JavaScriptCore/parser/Lexer.cpp:1571
>> +        return std::optional<Variant<double, const Identifier*>>(makeIdentifier(m_buffer8.data(), m_buffer8.size()));
> 
> Ditto.

Ditto.

>> Source/JavaScriptCore/runtime/JSBigInt.h:44
>> +    void initialize(bool zeroInitialize);
> 
> Pass some `enum class` instead of `bool`.

I agree!

[Caio Lima]

Created attachment 328091 [details]
Patch for test

I'm testing the EWS bots here. I still have some work to do on that from last review.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:150:  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:434:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:435:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:436:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:437:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 49 files


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

[Caio Lima]

Created attachment 328228 [details]
Benchmarks

Benchmarks looks neutral with this Patch implementation.

[Caio Lima]

Created attachment 328229 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:150:  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:434:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:435:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:436:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:437:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 49 files


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

[Caio Lima]

Created attachment 328448 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:150:  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:434:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:435:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:436:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:437:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 49 files


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

[Caio Lima]

Pong review

[Caio Lima]

Created attachment 328927 [details]
Patch

Rebasing with master.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:150:  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:434:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:435:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:436:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:437:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 51 files


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

[Caio Lima]

Ping review

[Darin Adler]

Comment on attachment 328927 [details]
Patch

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

There is a substantial number of tests here, but I am not sure they cover all the code. There is a lot of code!

> JSTests/ChangeLog:22
> +        * stress/big-int-literal-line-terminator.js: Added.
> +        (assert):
> +        * stress/big-int-literals.js: Added.
> +        (assert):
> +        (assertThrowSyntaxError):
> +        (catch):
> +        * stress/big-int-operations-error.js: Added.
> +        (assert):
> +        (assertThrowTypeError):
> +        * stress/big-int-type-of.js: Added.
> +        (assert):
> +        * stress/big-int-white-space-trailing-leading.js: Added.
> +        (assert):
> +        (assertThrowSyntaxError):

Although the script puts them in, I don’t think we need to put function lists for newly added files into the change log.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3149
> +    auto it = m_bigIntMap.find(entryKey);
> +    if (it != m_bigIntMap.end())
> +        return it->value;
> +    
> +    JSBigInt* bigIntInMap = JSBigInt::parseInt(nullptr, *vm(), identifier.string(), radix);
> +    // FIXME: [ESNext] Enables a way to throw an error on ByteCodeGenerator step
> +    // https://bugs.webkit.org/show_bug.cgi?id=180139
> +    RELEASE_ASSERT(bigIntInMap);
> +    addConstantValue(bigIntInMap);
> +
> +    auto addedEntry = m_bigIntMap.add(entryKey, bigIntInMap);
> +    ASSERT(addedEntry.isNewEntry);
> +
> +    return addedEntry.iterator->value;

This is double hashing, once in find and a second time in add. It’s more efficient to hash only once, and the best way to do that is normally to use the ensure function instead of a find/add pair. Can we do that?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1009
> +                return key.first->existingSymbolAwareHash() + WTF::intHash(key.second);

Adding two hashes together is not the best way to combine two hashes. Instead, please try this:

    return computeHash(key.first->existingSymbolAwareHash(), key.second);

That uses my newly added Hasher class so you might have to include Hasher.h.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1014
> +                return a.first == b.first && a.second == b.second;

This can just be "return a == b"; std::pair knows how to do ==.

> Source/JavaScriptCore/parser/Lexer.cpp:1560
> +        return std::optional<Variant<double, const Identifier*>>(binaryValue);

Should not need to be explicit about the std::optional part. This should work:

    return Variant<double, const Identifier*> { binaryValue };

Same thing in many other cases below.

> Source/JavaScriptCore/parser/Lexer.cpp:2168
> +            if (!parseNumberResult || WTF::holds_alternative<double>(*parseNumberResult))
> +                tokenData->doubleValue = parseNumberResult ? WTF::get<double>(*parseNumberResult) : 0;

I think this would be more elegant with two separate if statements rather than using the trinary to give us a zero.

Or possibly you could try one of these?

    auto parseNumberResult = parseBinary().value_or(0);
    auto parseNumberResult = parseBinary().value_or(0.0);

Not sure if that will compile without casting the 0 to the Variant type which would make it pretty ugly.

Same pattern recurs below.

> Source/JavaScriptCore/parser/Nodes.h:332
> +    class BigIntNode : public ConstantNode {

Could mark this class final. I think you should do it.

> Source/JavaScriptCore/parser/Nodes.h:339
> +        bool isBigInt() const override { return true; }
> +        JSValue jsValue(BytecodeGenerator&) const override;

We should mark these final instead of override, I think.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:80
> +        void* data = static_cast<void*>(dataStorage());
> +        memset(data, 0, length() * DigitSize);

No need for the static_cast, nor for the local variable. Can just pass dataStorage() to memset. Don’t forget to remove the unneeded braces too.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:119
> +std::optional<uint8_t> JSBigInt::toStringOneDigit()

I think a better name would be singleDigitValueForString. Calling it toString is not great because it returns the value for a digit, not a string, and not even a numeric digit character for a string, which is usually what "digit" means. Worse, Digit means something different in JSBigInt. Might want something in the name that makes it clear it’s base 10; not sure.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:371
> +        RELEASE_ASSERT(!(carry + high));

Why exactly does it need to be RELEASE_ASSERT instead of ASSERT here?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:429
> +// base-N string representation of a number. To increase accuracy, the array

I think you mean to increase precision?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:451
> +#if USE(JSVALUE64)
> +    int leadingZeros = clz64(lastDigit);
> +#else
> +    int leadingZeros = clz32(lastDigit);
> +#endif

Could the if here be based on sizeof(lastDigit) instead of USE? I think that would be clearer. The actual definition of Digit is based on uintptr_t, not on USE(JSVALUE64).

Or we could have an overloaded function so we don’t need an #if here at all.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:474
> +String JSBigInt::toStringGeneric(ExecState* exec, JSBigInt* x, int radix)

In new code I am suggesting we call this "state" rather than "exec", since the former is a noun and a word and the latter is a non-word and an adjective. In many cases we are using ExecState& instead of ExecState*, and I would have done that here.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:476
> +    StringVector<LChar> resultString;

Should one this down past the out-of-memory check. Also not 100% sure that we should use a StringVector and adopt it; OK for now I guess.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:490
> +        return StringImpl::adopt(WTFMove(resultString));

This should just return the null string; we don’t want to wastefully allocate memory when we are throwing an error.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:574
> +    Digit* data = trimmedBigInt->dataStorage();
> +    for (int i = 0; i < newLength; i++)
> +        data[i] = digit(i);

I would write this instead:

    std::copy(dataStorage(), dataStorage() + newLength, trimmedBigInt->dataStorage());

> Source/JavaScriptCore/runtime/JSBigInt.cpp:589
> +    size_t chars = static_cast<size_t>(charcount);

Should not require a static_cast.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:608
> +    if (exec)
> +        throwOutOfMemoryError(exec, scope);

What is this "if (exec)" about? Do we allow calling this with an exec of null?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:616
> +    JSBigInt* thisObject = jsCast<JSBigInt*>(cell);
> +    size_t bufferEstimatedSize = thisObject->m_length * DigitSize;
> +    return Base::estimatedSize(cell) + bufferEstimatedSize;

Seems like this works well as a one-liner:

    return Base::estimatedSize(cell) + jsCast<JSBigInt*>(cell)->m_length * digitSize;

> Source/JavaScriptCore/runtime/JSBigInt.h:59
> +public:
> +
> +    enum class InitializationType { None, WithZero };
> +    
> +    JSBigInt(VM&, Structure*, int length);
> +
> +    void initialize(InitializationType);
> +
> +    static void visitChildren(JSCell*, SlotVisitor&);
> +
> +    static size_t estimatedSize(JSCell*);
> +
> +    static size_t allocationSize(int length);
> +
> +    static Structure* createStructure(VM&, JSGlobalObject*, JSValue prototype);
> +
> +    static JSBigInt* createZero(ExecState*, VM&);
> +
> +    static JSBigInt* createWithLength(ExecState*, VM&, int length);

Some more grouping here would make this easier to read. For example:

    group InitializationType with initialize
    group estimatedSize with allocationSize
    group createZero with createWithLength
    group toStringOneDigit with toString

> Source/JavaScriptCore/runtime/JSBigInt.h:79
> +    JS_EXPORT_PRIVATE static bool equalToBigInt(JSBigInt* x, JSBigInt* y);

No need to name these arguments.

Not sure why we need to name this with the type in the name.

> Source/JavaScriptCore/runtime/JSBigInt.h:97
> +    static const int BitsPerByte = 8;
> +    static const int DigitSize = sizeof(Digit);
> +    static const int DigitBits = DigitSize * BitsPerByte;
> +    static const int HalfDigitBits = DigitBits / 2;
> +    static const Digit HalfDigitMask = (1ull << HalfDigitBits) - 1;
> +    static const int MaxInt = 0x7FFFFFFF;
> +    
> +    // The maximum length that the current implementation supports would be
> +    // MaxInt / DigitBits. However, we use a lower limit for now, because
> +    // raising it later is easier than lowering it.
> +    // Support up to 1 million bits.
> +    static const int MaxLength = 1024 * 1024 / (sizeof(void*) * BitsPerByte);

WebKit coding style does not use capital letters for constants; we use that for types. These should probably all be constexpr.

Also seems like sizeof(Digit) is clearer than DigitSize, so not 100% sure we should have a named constant for that one.

> Source/JavaScriptCore/runtime/JSBigInt.h:107
> +    static Digit digitAdd(Digit a, Digit b, Digit& carry);
> +    static Digit digitSub(Digit a, Digit b, Digit& borrow);
> +    static Digit digitMul(Digit a, Digit b, Digit& high);

No need for the "a" and "b" names here.

> Source/JavaScriptCore/runtime/JSBigInt.h:116
> +    template <typename CharType>
> +    static JSBigInt* parseInt(ExecState* exec, VM& vm, CharType* data, int length, int startIndex, int radix, bool allowEmptyString = true)

Can this go into the cpp file please? We can declare the template in the header and only define it in the source file as long as it’s only used inside the source file.

> Source/JavaScriptCore/runtime/JSBigInt.h:177
> +    static size_t offsetOfData()
> +    {
> +        return WTF::roundUpToMultipleOf<sizeof(Digit)>(sizeof(JSBigInt));
> +    }

If these private functions are not used outside the .cpp file, please move them inside to cut down on recompilation and for clarity.

> Source/JavaScriptCore/runtime/JSBigInt.h:181
> +        return bitwise_cast<Digit*>(bitwise_cast<char*>(this) + offsetOfData());

There is no need to use bitwise_cast here. Should use reinterpret_cast in a case like this where it’s just about different pointer types. We should reserve bitwise_cast for cases where its different semantic is needed.

> Source/JavaScriptCore/runtime/JSBigInt.h:188
> +        Digit* dataPtr = dataStorage();
> +        return dataPtr[n];

No need for the local variable here.

> Source/JavaScriptCore/runtime/JSBigInt.h:195
> +        Digit* digitPtr = dataStorage();
> +        digitPtr[n] = value;

Ditto.

> Source/JavaScriptCore/runtime/JSBigInt.h:200
> +        return length * DigitSize;

What guarantees we can’t overflow here?

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:386
> +        std::optional<uint8_t> digit = bigInt->toStringOneDigit();
> +        if (digit)
> +            return vm.smallStrings.singleCharacterString(*digit + '0');

Better to declare inside the if statement:

    if (auto digit = bigInt->toStringOneDigit())

[Caio Lima]

Comment on attachment 328927 [details]
Patch

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

Thank you very much for the review and suggestions!

About tests, there are pieces of code here that are prepared to followup bug https://bugs.webkit.org/show_bug.cgi?id=175359, such as toString, parseInt(to be named to another method as discussed into BigInt proposal) and BigInt constructor that requires some machinery that I removed to simplify review process. Also, there are plenty of patches with binary/unary operations that will insert another amount of tests as well.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1009
>> +                return key.first->existingSymbolAwareHash() + WTF::intHash(key.second);
> 
> Adding two hashes together is not the best way to combine two hashes. Instead, please try this:
> 
>     return computeHash(key.first->existingSymbolAwareHash(), key.second);
> 
> That uses my newly added Hasher class so you might have to include Hasher.h.

Thx!

>> Source/JavaScriptCore/parser/Lexer.cpp:1560
>> +        return std::optional<Variant<double, const Identifier*>>(binaryValue);
> 
> Should not need to be explicit about the std::optional part. This should work:
> 
>     return Variant<double, const Identifier*> { binaryValue };
> 
> Same thing in many other cases below.

That's much better. Changed.

>> Source/JavaScriptCore/parser/Lexer.cpp:2168
>> +                tokenData->doubleValue = parseNumberResult ? WTF::get<double>(*parseNumberResult) : 0;
> 
> I think this would be more elegant with two separate if statements rather than using the trinary to give us a zero.
> 
> Or possibly you could try one of these?
> 
>     auto parseNumberResult = parseBinary().value_or(0);
>     auto parseNumberResult = parseBinary().value_or(0.0);
> 
> Not sure if that will compile without casting the 0 to the Variant type which would make it pretty ugly.
> 
> Same pattern recurs below.

ok

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:119
>> +std::optional<uint8_t> JSBigInt::toStringOneDigit()
> 
> I think a better name would be singleDigitValueForString. Calling it toString is not great because it returns the value for a digit, not a string, and not even a numeric digit character for a string, which is usually what "digit" means. Worse, Digit means something different in JSBigInt. Might want something in the name that makes it clear it’s base 10; not sure.

Yes, I agree. Also calling using Digit to refers to either base (2^32) or (2^64) sounds poor to me, but I also can't think in any other better name. Following your comment.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:371
>> +        RELEASE_ASSERT(!(carry + high));
> 
> Why exactly does it need to be RELEASE_ASSERT instead of ASSERT here?

I would like to cause a break due to wrong configuration, but I agree that we should use ASSERT.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:429
>> +// base-N string representation of a number. To increase accuracy, the array
> 
> I think you mean to increase precision?

The original comment is not mine. It is from original V8 code. I understand precision as you mentioned.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:451
>> +#endif
> 
> Could the if here be based on sizeof(lastDigit) instead of USE? I think that would be clearer. The actual definition of Digit is based on uintptr_t, not on USE(JSVALUE64).
> 
> Or we could have an overloaded function so we don’t need an #if here at all.

Using "if" here.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:476
>> +    StringVector<LChar> resultString;
> 
> Should one this down past the out-of-memory check. Also not 100% sure that we should use a StringVector and adopt it; OK for now I guess.

FIXME added here. https://bugs.webkit.org/show_bug.cgi?id=18067

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:490
>> +        return StringImpl::adopt(WTFMove(resultString));
> 
> This should just return the null string; we don’t want to wastefully allocate memory when we are throwing an error.

Done.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:608
>> +        throwOutOfMemoryError(exec, scope);
> 
> What is this "if (exec)" about? Do we allow calling this with an exec of null?

We don't have exec at "BytecodeGenerator::addBigIntConstant". Probably we will remove it when fixing https://bugs.webkit.org/show_bug.cgi?id=180139

>> Source/JavaScriptCore/runtime/JSBigInt.h:116
>> +    static JSBigInt* parseInt(ExecState* exec, VM& vm, CharType* data, int length, int startIndex, int radix, bool allowEmptyString = true)
> 
> Can this go into the cpp file please? We can declare the template in the header and only define it in the source file as long as it’s only used inside the source file.

Sure.

>> Source/JavaScriptCore/runtime/JSBigInt.h:200
>> +        return length * DigitSize;
> 
> What guarantees we can’t overflow here?

Nothing, but we aren't using this method anymore. Removing it.

[Caio Lima]

Created attachment 329078 [details]
Patch for landing

Asking for new review before landing cause I added new test and changed code based on comments.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:147:  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:411:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:429:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:430:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:431:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:432:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:111:  The parameter name "state" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:111:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:122:  The parameter name "n" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:123:  The parameter name "n" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: JSTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:50:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 12 in 52 files


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

[Darin Adler]

Comment on attachment 329078 [details]
Patch for landing

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1019
> +        struct BigIntEntryHash {
> +            static unsigned hash(BigIntMapEntry key)
> +            {
> +                return computeHash(key.first->existingSymbolAwareHash(), key.second);
> +            }
> +            
> +            static bool equal(BigIntMapEntry a, BigIntMapEntry b)
> +            {
> +                return a == b;
> +            }
> +            
> +            static const bool safeToCompareToEmptyOrDeleted = true;
> +        };

Now that I look at this again, I believe the default hash for pairs will do the right thing without us coding anything here. We just not specify BigIntEntryHash and let the default do its thing. We could also remove the Hasher.h include if we do that.

> Source/JavaScriptCore/parser/Lexer.h:181
> +    ALWAYS_INLINE Variant<double, const Identifier*> parseHex();
> +    ALWAYS_INLINE std::optional<Variant<double, const Identifier*>> parseBinary();
> +    ALWAYS_INLINE std::optional<Variant<double, const Identifier*>> parseOctal();
> +    ALWAYS_INLINE std::optional<Variant<double, const Identifier*>> parseDecimal();

Another way to make the code tighter is to do this:

    using NumberParseResult = Variant<double, const Identifier*>;

Then you can just use that instead of writing out the Variant type each time. Like:

    ALWAYS_INLINE NumberParseResult parseHex();
    ALWAYS_INLINE std::optional<NumberParseResult> parseBinary();

And:

    template<typename T> ALWAYS_INLINE auto Lexer<T>::parseBinary() -> std::optional<NumberParseResult>

> Source/JavaScriptCore/parser/Nodes.h:338
> +        bool isBigInt() const override { return true; }

This can be final instead of override.

[Caio Lima]

Created attachment 329106 [details]
Patch for landingh

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:147:  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:428:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:429:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:430:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.cpp:431:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/HashFunctions.h:221:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 54 files


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

[WebKit Commit Bot]

Comment on attachment 329106 [details]
Patch for landingh

Clearing flags on attachment: 329106

Committed r225799: <https://trac.webkit.org/changeset/225799>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36002172>

[Joseph Pecoraro]

Comment on attachment 329106 [details]
Patch for landingh

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

> JSTests/bigIntTests.yaml:6
> +# 1. Redistributions of source code must retain the above copyright #    notice, this list of conditions and the following disclaimer.

Missing newline.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:472
> +    // https://bugs.webkit.org/show_bug.cgi?id=18067

This doesn't look like the right bugzilla bug. You want:
https://bugs.webkit.org/show_bug.cgi?id=180671

I'd like to see tests for toString. The radix behavior is special and also there is specific `this` value behavior:
https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring

> The toString function is not generic; it throws a TypeError exception if its this value is not a BigInt or a BigInt object. Therefore, it cannot be transferred to other kinds of objects for use as a method.

Likewise JSON.stringify tests for BigInt, as you pointed out on IRC it is stringifyable.

[Caio Lima]

(In reply to Joseph Pecoraro from comment #62)
> Comment on attachment 329106 [details]
> Patch for landingh
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329106&action=review
> 
> > JSTests/bigIntTests.yaml:6
> > +# 1. Redistributions of source code must retain the above copyright #    notice, this list of conditions and the following disclaimer.
> 
> Missing newline.

Oh, Nice catch. I will fix that into followup bug: https://bugs.webkit.org/show_bug.cgi?id=175359

> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:472
> > +    // https://bugs.webkit.org/show_bug.cgi?id=18067
> 
> This doesn't look like the right bugzilla bug. You want:
> https://bugs.webkit.org/show_bug.cgi?id=180671

Ditto.

> I'd like to see tests for toString. The radix behavior is special and also
> there is specific `this` value behavior:
> https://tc39.github.io/proposal-bigint/#sec-bigint.prototype.tostring
> 
> > The toString function is not generic; it throws a TypeError exception if its this value is not a BigInt or a BigInt object. Therefore, it cannot be transferred to other kinds of objects for use as a method.
> 
> Likewise JSON.stringify tests for BigInt, as you pointed out on IRC it is
> stringifyable.

The current "JSBigInt::toString" implementation we have is just used by JSC internal print. If you try ```let a = 10n; a.toString()```, it will crash because we don't have BigIntPrototype in current Patch. This is going to be added in https://bugs.webkit.org/show_bug.cgi?id=175359 as well.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #59)
> Comment on attachment 329106 [details]
> Patch for landingh
> 
> Clearing flags on attachment: 329106
> 
> Committed r225799: <https://trac.webkit.org/changeset/225799>
The tests added with this change are failing an assertion on 32-bit JSC bots:
https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/706/steps/webkit-32bit-jsc-test/logs/stdio

stress/big-int-function-apply.js.big-int-enabled: ERROR: Unchecked JS exception:
stress/big-int-function-apply.js.big-int-enabled:     This scope can throw a JS exception: allocateFor @ ./runtime/JSBigInt.cpp:581
stress/big-int-function-apply.js.big-int-enabled:         (ExceptionScope::m_recursionDepth was 4)
stress/big-int-function-apply.js.big-int-enabled:     But the exception was unchecked as of this scope: getNonIndexPropertySlot @ /Volumes/Data/slave/highsierra-32bitJSC-debug/build/Source/JavaScriptCore/runtime/JSObjectInlines.h:136
stress/big-int-function-apply.js.big-int-enabled:         (ExceptionScope::m_recursionDepth was 4)
stress/big-int-function-apply.js.big-int-enabled: 
stress/big-int-function-apply.js.big-int-enabled: Unchecked exception detected at:
stress/big-int-function-apply.js.big-int-enabled:     1   0x12a6a5d JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
stress/big-int-function-apply.js.big-int-enabled:     2   0x127ebb6 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
stress/big-int-function-apply.js.big-int-enabled:     3   0x127d98b JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
stress/big-int-function-apply.js.big-int-enabled:     4   0x365976 JSC::JSObject::getNonIndexPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
stress/big-int-function-apply.js.big-int-enabled:     5   0x36532a JSC::JSObject::getPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
stress/big-int-function-apply.js.big-int-enabled:     6   0x1115aa5 JSC::JSObject::hasPropertyGeneric(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot::InternalMethodType) const
stress/big-int-function-apply.js.big-int-enabled:     7   0x1115a07 JSC::JSObject::hasProperty(JSC::ExecState*, JSC::PropertyName) const
stress/big-int-function-apply.js.big-int-enabled:     8   0xcce4c8 JSC::JSGlobalObject::addVar(JSC::ExecState*, JSC::Identifier const&)
stress/big-int-function-apply.js.big-int-enabled:     9   0x11e62e3 JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*)
stress/big-int-function-apply.js.big-int-enabled:     10  0xccdc49 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
stress/big-int-function-apply.js.big-int-enabled:     11  0xfaf0e2 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
stress/big-int-function-apply.js.big-int-enabled:     12  0x106c71 runWithOptions(GlobalObject*, CommandLine&)
stress/big-int-function-apply.js.big-int-enabled:     13  0xdbf9c jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*) const
stress/big-int-function-apply.js.big-int-enabled:     14  0xc26af int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
stress/big-int-function-apply.js.big-int-enabled:     15  0xc0e70 jscmain(int, char**)
stress/big-int-function-apply.js.big-int-enabled:     16  0xc0d97 main
stress/big-int-function-apply.js.big-int-enabled:     17  0xa75536e1 start
stress/big-int-function-apply.js.big-int-enabled:     18  0x9

[Ryan Haddad]

(In reply to Ryan Haddad from comment #64)
> (In reply to WebKit Commit Bot from comment #59)
> > Comment on attachment 329106 [details]
> > Patch for landingh
> > 
> > Clearing flags on attachment: 329106
> > 
> > Committed r225799: <https://trac.webkit.org/changeset/225799>
> The tests added with this change are failing an assertion on 32-bit JSC bots:
> https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/706/steps/webkit-32bit-jsc-test/logs/
> stdio

Correction: this occurs on debug JSC bots, not just 32-bit ones.
https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1706

[Caio Lima]

(In reply to Ryan Haddad from comment #65)
> (In reply to Ryan Haddad from comment #64)
> > (In reply to WebKit Commit Bot from comment #59)
> > > Comment on attachment 329106 [details]
> > > Patch for landingh
> > > 
> > > Clearing flags on attachment: 329106
> > > 
> > > Committed r225799: <https://trac.webkit.org/changeset/225799>
> > The tests added with this change are failing an assertion on 32-bit JSC bots:
> > https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/706/steps/webkit-32bit-jsc-test/logs/
> > stdio
> 
> Correction: this occurs on debug JSC bots, not just 32-bit ones.
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1706

Thanks for the notification. I'm handling this bug into https://bugs.webkit.org/show_bug.cgi?id=180746

[Caio Lima]

(In reply to Ryan Haddad from comment #65)
> (In reply to Ryan Haddad from comment #64)
> > (In reply to WebKit Commit Bot from comment #59)
> > > Comment on attachment 329106 [details]
> > > Patch for landingh
> > > 
> > > Clearing flags on attachment: 329106
> > > 
> > > Committed r225799: <https://trac.webkit.org/changeset/225799>
> > The tests added with this change are failing an assertion on 32-bit JSC bots:
> > https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/706/steps/webkit-32bit-jsc-test/logs/
> > stdio
> 
> Correction: this occurs on debug JSC bots, not just 32-bit ones.
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20JSC%20%28Tests%29/builds/1706

Patch with fix into https://bugs.webkit.org/show_bug.cgi?id=180746