[webkit-reviews] review denied: [Bug 179000] [ESNext][BigInt] Implement BigInt literals and JSBigInt : [Attachment 327346] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has denied Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 179000: [ESNext][BigInt] Implement BigInt literals and JSBigInt
https://bugs.webkit.org/show_bug.cgi?id=179000

Attachment 327346: Patch

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




--- Comment #15 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 327346
  --> https://bugs.webkit.org/attachment.cgi?id=327346
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.