Array.prototype native functions should use Symbol.species to construct the result

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Array.prototype native functions should use Symbol.species to construct the result

[Keith Miller]

Created attachment 270235 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:197:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:202:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:206:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 8 files


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

[Keith Miller]

Created attachment 270237 [details]
Benchmark results

[Saam Barati]

Comment on attachment 270235 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:181
> +    JSValue constructor = jsUndefined();

Might be worth a spec-link here.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:193
> +        // FIXME: this should be IsConstructor

it's worth creating a bug for this or using it if we have it.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:201
> +            if (exec->hadException() || constructor.isNull())

These should be split out.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:621
> +        // We add the newTarget because the compiler gets confused between 0 being a number and a pointer.

you could probably cast your arguments.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1050
> +

style: too many newlines.

[Saam Barati]

Comment on attachment 270235 [details]
Patch

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

> Source/JavaScriptCore/tests/stress/array-species-functions.js:58
> +    let called = false;
> +    function species(...args) {
> +        called = true;
> +        return new C(...args);
> +    }

might be worth having a species that throws as a test case.

[Keith Miller]

Created attachment 270262 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:197:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:204:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:208:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 10 files


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

[Keith Miller]

Whoops meant to pretty-diff not land-safely...

[Keith Miller]

Comment on attachment 270235 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:181
>> +    JSValue constructor = jsUndefined();
> 
> Might be worth a spec-link here.

Seems reasonable.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:193
>> +        // FIXME: this should be IsConstructor
> 
> it's worth creating a bug for this or using it if we have it.

I don't see a function that does this I'm going to add a basic one with a bug to make it better. The code for it is:

inline bool JSValue::isConstructor() const
{
    if (isFunction()) {
        ConstructData data;
        return getConstructData(*this, data) != ConstructTypeNone;
    }
    return false;
}

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:201
>> +            if (exec->hadException() || constructor.isNull())
> 
> These should be split out.

Fixed.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:621
>> +        // We add the newTarget because the compiler gets confused between 0 being a number and a pointer.
> 
> you could probably cast your arguments.

Actually the last to arguments are optional and remove this issue. So, it's now constructEmptyArray(exec, nullptr)

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1050
>> +
> 
> style: too many newlines.

Fixed.

>> Source/JavaScriptCore/tests/stress/array-species-functions.js:58
>> +    }
> 
> might be worth having a species that throws as a test case.

Fixed.

[Keith Miller]

Created attachment 270265 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 270265 [details]
Patch for landing

Clearing flags on attachment: 270265

Committed r195878: <http://trac.webkit.org/changeset/195878>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.