[webkit-reviews] review granted: [Bug 209782] [ECMA-402] Properly implement BigInt.prototype.toLocaleString : [Attachment 396134] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 209782: [ECMA-402] Properly implement BigInt.prototype.toLocaleString
https://bugs.webkit.org/show_bug.cgi?id=209782

Attachment 396134: Patch

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




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 396134
  --> https://bugs.webkit.org/attachment.cgi?id=396134
Patch

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

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> +	   return throwVMTypeError(globalObject, scope, "'this' value must be a
BigInt or BigIntObject"_s);

Do we have test cases for this?

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:126
> +	   return throwVMTypeError(globalObject, scope, "'this' value must be a
BigInt or BigIntObject"_s);

I don’t see any test cases for this.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:128
> +    IntlNumberFormat* numberFormat = IntlNumberFormat::create(vm,
globalObject->numberFormatStructure());

JavaScriptCore is a whole other world, but if I was writing this in elsewhere
in WebKIt I would suggest auto rather  than writing the typename here.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:374
> +    auto string = value->toString(globalObject, 10);
> +    RETURN_IF_EXCEPTION(scope, { });
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    Vector<UChar, 32> buffer(32);
> +    auto length = unum_formatDecimal(m_numberFormat.get(),
string.utf8().data(), string.length(), buffer.data(), buffer.size(), nullptr,
&status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +	   buffer.grow(length);
> +	   status = U_ZERO_ERROR;
> +	   unum_formatDecimal(m_numberFormat.get(), string.utf8().data(),
string.length(), buffer.data(), length, nullptr, &status);
> +    }

Seems wasteful here to first make a WTF::String that is guaranteed to be all
ASCII, then call UTF-8, allocating a WTF::CString, and even doing it a second
time if the result is more than 32 characters.

It’s not typical to write WTF::String code that depends on LChar vs. UChar, but
I think here we could use characters8() here because we are guaranteed that
JSBigInt::toString returns a Latin-1 string.

Also, it’s a bad pattern to use string.utf8().data() with string.length(); if
the string had any non-ASCII characters, then the length would be the length of
the Latin-1, not the length of the UTF-8 (which is longer). Irrelevant, since
we should not be using utf8().

If we cared about performance we could make a function on JSBigInt that puts
the data into a vector with inline capacity instead of a string. Related to bug
18067.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:376
> +	   return throwException(globalObject, scope, createError(globalObject,
"Failed to format a BigInt."_s));

I don’t see any test cases for this. Maybe because there is actually no real
case where this could fail, and this is effectively dead code?