Array.prototype native functions should use Symbol.species to construct the result
Created attachment 270235 [details] Patch
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.
Created attachment 270237 [details] Benchmark results
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.
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.
Created attachment 270262 [details] Patch for landing
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.
Whoops meant to pretty-diff not land-safely...
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.
Created attachment 270265 [details] Patch for landing
Comment on attachment 270265 [details] Patch for landing Clearing flags on attachment: 270265 Committed r195878: <http://trac.webkit.org/changeset/195878>
All reviewed patches have been landed. Closing bug.