[webkit-reviews] review granted: [Bug 182434] [JSC] Clean up ArraySpeciesCreate : [Attachment 373352] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 182434: [JSC] Clean up ArraySpeciesCreate
https://bugs.webkit.org/show_bug.cgi?id=182434

Attachment 373352: Patch

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




--- Comment #13 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 373352
  --> https://bugs.webkit.org/attachment.cgi?id=373352
Patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:13
> +

Let's paste the measurement result here to tell that this is measured and it is
not intended to cause the performance regression.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:242
> +		   && jsDynamicCast<ArrayConstructor*>(vm, constructorObject);

Use `inherit<ArrayConstructor>` instead.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:272
> +    JSObject* object = asObject(exec->argument(0));

Let's use uncheckedArgument(0) because we know all the caller of
arrayProtoFuncSpeciesCreate passes the first argument.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:273
> +    auto length = static_cast<uint64_t>(exec->argument(1).asNumber());

Use `uint64_t` instead of `auto` here. JSC codebase likes writing a type
explicitly.
And use `uncheckedArgument(1)` too.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:278
> +	   return encodedJSValue();

Use `{}` for the newer code.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:283
> +    auto unsignedLength = static_cast<unsigned>(length);
> +    if (unsignedLength != length) {

Comparing with `std::numeric_limits<unsigned>::max()` is better since it
describes the intent explicitly.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:285
> +	   return encodedJSValue();

Ditto.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:290
> +    JSObject* result = constructEmptyArray(exec, nullptr, unsignedLength);
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    RELEASE_AND_RETURN(scope, JSValue::encode(result));

Do things like,

RELEASE_AND_RETURN(scope, JSValue::encode(constructEmptyArray(exec, nullptr,
unsignedLength)));

The L289's exception check is not necessary if we write this code like the
above one.