[webkit-reviews] review granted: [Bug 175359] [ESNext][BigInt] Implement BigIntConstructor and BigIntPrototype : [Attachment 329726] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 21 11:45:24 PST 2017
Yusuke Suzuki <utatane.tea at gmail.com> has granted Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 175359: [ESNext][BigInt] Implement BigIntConstructor and BigIntPrototype
https://bugs.webkit.org/show_bug.cgi?id=175359
Attachment 329726: Patch
https://bugs.webkit.org/attachment.cgi?id=329726&action=review
--- Comment #32 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 329726
--> https://bugs.webkit.org/attachment.cgi?id=329726
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=329726&action=review
r=me with comments.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:20
> + */
I think new code in WebKit uses 2-clauses BSD license.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:46
> +const ClassInfo BigIntConstructor::s_info = { "Function",
&InternalFunction::s_info, &bigIntConstructorTable, nullptr,
CREATE_METHOD_TABLE(BigIntConstructor) };
`&Base::s_info` is better than `&InternalFunction::s_info`.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:79
> + if (argument.isInt32())
> + isInteger = true;
Let's use early returns. Then, we do not need to have `bool isInteger`.
Like,
if (argument.isInt32())
return true;
if (!argument.isDouble())
return false;
...
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:103
> + JSBigInt* result = JSBigInt::createFrom(vm, argument.asBoolean());
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +
> + return JSValue::encode(result);
Let's just return JSBigInt::createFrom result directly. Like,
if (argument.isBoolean()) {
scope.release();
return JSValue::encode(JSBigInt::createFrom(vm, argument.asBoolean()));
}
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:107
> + return throwVMTypeError(&state, scope);
Let's add an error message.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:116
> + JSBigInt* bigInt = JSBigInt::parseInt(&state, view);
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +
> + if (!bigInt)
> + return throwVMError(&state, scope, createSyntaxError(&state,
"Failed to parse String to BigInt"));
Why not throwing SyntaxError inside parseInt? Then, we do not need to have
if (!bigInt)
...
here. And we can just write this code as,
return JSBigInt::parseInt(&state, view);
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:120
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> + return result;
Let's return `toStringView()` result directly.
scope.release();
return toStringView(...);
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:129
> +static EncodedJSValue JSC_HOST_CALL constructBigIntConstructor(ExecState*
state)
> +{
> + VM& vm = state->vm();
> + auto scope = DECLARE_THROW_SCOPE(vm);
> +
> + return throwVMTypeError(state, scope);
> +}
If you do not have [[Construct]] in BigInt, we should not define this function.
Just passing `nullptr` to InternalFunction constructor. (See SymbolConstructor
implementation).
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:147
> + JSBigInt* result = JSBigInt::createFrom(vm,
primitive.asInt32());
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> + return JSValue::encode(result);
Return `JSValue::encode(JSBigInt::createFrom(...))` directly.
scope.release();
return JSValue::encode(JSBigInt::createFrom(...));
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:153
> + JSBigInt* result = JSBigInt::createFrom(vm,
primitive.asUInt32());
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> + return JSValue::encode(result);
Ditto.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:158
> + JSBigInt* result = JSBigInt::createFrom(vm,
static_cast<int64_t>(primitive.asDouble()));
> + RETURN_IF_EXCEPTION(scope, encodedJSValue());
> + return JSValue::encode(result);
Ditto.
> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:176
> +EncodedJSValue JSC_HOST_CALL bigIntConstructorFuncAsUintN(ExecState*)
> +{
> + CRASH();
> + return JSValue::encode(JSValue());
> +}
> +
> +EncodedJSValue JSC_HOST_CALL bigIntConstructorFuncAsIntN(ExecState*)
> +{
> + CRASH();
> + return JSValue::encode(JSValue());
> +}
Let's add a FIXME comment with URL.
> Source/JavaScriptCore/runtime/BigIntConstructor.h:19
> + */
Ditto
> Source/JavaScriptCore/runtime/BigIntConstructor.h:32
> + typedef InternalFunction Base;
Use `using` in new code.
> Source/JavaScriptCore/runtime/BigIntObject.cpp:36
> +const ClassInfo BigIntObject::s_info = { "BigInt", &JSWrapperObject::s_info,
nullptr, nullptr, CREATE_METHOD_TABLE(BigIntObject) };
Use `&Base::s_info` instead.
> Source/JavaScriptCore/runtime/BigIntObject.h:20
> + */
Ditto
> Source/JavaScriptCore/runtime/BigIntObject.h:31
> + typedef JSWrapperObject Base;
Use `using` instead.
> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:20
> + */
Ditto
> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:73
> +static ALWAYS_INLINE bool toThisBigIntValue(VM& vm, JSValue thisValue,
JSBigInt** x)
I think using `jsDynamicCast`'s idiom is preferable to JSC code.
Let's return `JSBigInt*`. If we cannot find it, just return nullptr.
> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> + JSValue radixValue = state->argument(0);
> + int32_t radix;
> + if (radixValue.isInt32())
> + radix = radixValue.asInt32();
> + else if (radixValue.isUndefined())
> + radix = 10;
> + else
> + radix = static_cast<int32_t>(radixValue.toInteger(state)); // nan ->
0
> +
> + return radix;
Let's use early return.
> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:112
> + JSBigInt* value = nullptr;
> + if (!toThisBigIntValue(vm, state->thisValue(), &value))
> + return throwVMTypeError(state, scope, ASCIILiteral("'this' value
must be a BigInt or BigIntObject"));
Then, we can write this as,
JSBigInt* value = toThisBigIntValue(vm, state->thisValue());
if (!value)
return throwVMTypeError(state, scope, ASCIILiteral("'this' value must be a
BigInt or BigIntObject"));
> Source/JavaScriptCore/runtime/BigIntPrototype.h:19
> + */
Ditto
> Source/JavaScriptCore/runtime/BigIntPrototype.h:29
> + typedef JSNonFinalObject Base;
Use `using` in new code.
> Source/JavaScriptCore/runtime/IntlCollator.cpp:263
> + auto result = resolveLocale(state, availableLocales, requestedLocales,
opt, relevantCollatorExtensionKeys,
WTF_ARRAY_LENGTH(relevantCollatorExtensionKeys), localeData);
I think this file is not related to this patch.
> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:197
> + auto result = resolveLocale(state, availableLocales, requestedLocales,
opt, relevantNumberExtensionKeys,
WTF_ARRAY_LENGTH(relevantNumberExtensionKeys), IntlNFInternal::localeData);
Ditto.
> Source/JavaScriptCore/runtime/JSBigInt.cpp:124
> + bigInt->setDigit(0, static_cast<Digit>(-1 * tempValue));
Let's just write `bigInt->setDigit(0, static_cast<Digit>(-1 *
static_cast<int64_t>(value)));`
> Source/JavaScriptCore/runtime/JSBigInt.cpp:153
> + uint64_t tempValue = static_cast<uint64_t>(-(value + 1)) + 1;
> + bigInt->setDigit(0, static_cast<Digit>(tempValue));
Let's just write `bigInt->setDigit(0,
static_cast<Digit>(static_cast<uint64_t>(-(value + 1)) + 1)));`.
> Source/JavaScriptCore/runtime/JSBigInt.cpp:173
> + uint64_t tempValue;
> + bool sign = false;
> + if (value < 0) {
> + tempValue = static_cast<uint64_t>(-(value + 1)) + 1;
> + sign = true;
> + } else
> + tempValue = value;
> +
> + Digit lowBits = static_cast<Digit>(value & 0xffffffff);
> + Digit highBits = static_cast<Digit>((value >> 32) & 0xffffffff);
> +
> + bigInt->setDigit(0, lowBits);
> + bigInt->setDigit(1, highBits);
> + bigInt->setSign(sign);
I think this has a bug since nobody uses tempValue. Could you add a test for
this?
> Source/JavaScriptCore/runtime/JSBigInt.cpp:735
> + if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'b'))
> + return parseInt(exec, vm, data, length, p + 2, 2, false);
> +
> + if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'x'))
> + return parseInt(exec, vm, data, length, p + 2, 16, false);
> +
> + if (data[p] == '0' && isASCIIAlphaCaselessEqual(data[p + 1], 'o'))
> + return parseInt(exec, vm, data, length, p + 2, 8, false);
Let's combine this `data[p] == '0'` check.
> Source/JavaScriptCore/runtime/JSCPoisonedPtr.cpp:28
> +#include <mutex>
Why is this necessary?
More information about the webkit-reviews
mailing list