Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1286)

Issue 771483002: Implement ES6 @@isConcatSpreadable (Closed)

Created:
6 years ago by caitp (gmail)
Modified:
6 years ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Project:
v8
Visibility:
Public.

Description

Implement ES6 @@isConcatSpreadable / Array.prototype.concat Add support for Symbol.isConcatSpreadable in Array.prototype.concat. This enables spreading non-Array objects with the symbol. LOG=N R=dslomov@chromium.org BUG=

Patch Set 1 #

Patch Set 2 : Move implementation to C++ to fix speed issue #

Total comments: 7

Patch Set 3 : Uncomment test cases (should have been done in patch set #2) #

Patch Set 4 : Rebase #

Total comments: 2

Patch Set 5 : Add TypedArray tests to ensure they are not spread #

Patch Set 6 : Set @@isConcatSpreadable on TypedArrays in tests #

Total comments: 4

Patch Set 7 : Add tests for sloppy arguments + isConcatSpreadable #

Total comments: 4

Patch Set 8 : Add SLOPPY_ARGUMENTS_ELEMENTS handler to IterateElements, additional tests #

Patch Set 9 : Fix holey sloppy-arguments case #

Total comments: 1

Patch Set 10 : Remove default case from IterateElements(), add missing Fixed<Type>Array cases #

Total comments: 4

Patch Set 11 : Rename IterateExternalArrayElements, add tests which hit Fixed<Type>Array cases #

Unified diffs Side-by-side diffs Delta from patch set Stats (+546 lines, -21 lines) Patch
M src/harmony-array.js View 1 1 chunk +11 lines, -0 lines 0 comments Download
M src/runtime.js View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
M src/runtime/runtime-array.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +113 lines, -21 lines 0 comments Download
A test/mjsunit/harmony/array-concat.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +413 lines, -0 lines 0 comments Download

Messages

Total messages: 25 (2 generated)
caitp (gmail)
As mentioned in the CL description, I believe the draft's behaviour results in a quite ...
6 years ago (2014-11-30 20:32:08 UTC) #1
Dmitry Lomov (no reviews)
On 2014/11/30 20:32:08, caitp wrote: > As mentioned in the CL description, I believe the ...
6 years ago (2014-11-30 21:32:54 UTC) #2
caitp (gmail)
On 2014/11/30 21:32:54, Dmitry Lomov (chromium) wrote: > On 2014/11/30 20:32:08, caitp wrote: > > ...
6 years ago (2014-11-30 21:41:56 UTC) #3
caitp (gmail)
On 2014/11/30 21:41:56, caitp wrote: > On 2014/11/30 21:32:54, Dmitry Lomov (chromium) wrote: > > ...
6 years ago (2014-12-02 04:31:55 UTC) #4
caitp (gmail)
I've re-implemented a subset of this CL in C++, as the JS implementation was unfortunately ...
6 years ago (2014-12-02 15:37:34 UTC) #5
Dmitry Lomov (no reviews)
You don't have to worry about Array subclassing just yet - it does not work ...
6 years ago (2014-12-10 10:58:21 UTC) #7
caitp (gmail)
https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/array-concat.js File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/20001/test/mjsunit/harmony/array-concat.js#newcode10 test/mjsunit/harmony/array-concat.js:10: assertEquals(1, Array.prototype.concat.length); On 2014/12/10 10:58:21, Dmitry Lomov (chromium) wrote: ...
6 years ago (2014-12-10 13:25:33 UTC) #8
Dmitry Lomov (no reviews)
Sorry for raising false alarms on FAST_SMI_ELEMENTS and friends, these are only applicable for JSArrays ...
6 years ago (2014-12-10 14:42:00 UTC) #9
caitp (gmail)
https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/array-concat.js File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/array-concat.js#newcode9 test/mjsunit/harmony/array-concat.js:9: (function testArrayConcatArity() { On 2014/12/10 14:41:59, Dmitry Lomov (chromium) ...
6 years ago (2014-12-10 15:29:20 UTC) #10
Dmitry Lomov (no reviews)
On 2014/12/10 15:29:20, caitp wrote: > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/array-concat.js > File test/mjsunit/harmony/array-concat.js (right): > > https://codereview.chromium.org/771483002/diff/60001/test/mjsunit/harmony/array-concat.js#newcode9 > ...
6 years ago (2014-12-10 16:02:38 UTC) #11
caitp (gmail)
On 2014/12/10 16:02:38, Dmitry Lomov (chromium) wrote: > On 2014/12/10 15:29:20, caitp wrote: > > ...
6 years ago (2014-12-10 16:03:41 UTC) #12
Dmitry Lomov (no reviews)
lgtm % nit https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/array-concat.js File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/100001/test/mjsunit/harmony/array-concat.js#newcode129 test/mjsunit/harmony/array-concat.js:129: testConcatTypedArray(ctor, 150, max[i]); Make this smaller, ...
6 years ago (2014-12-10 16:36:37 UTC) #13
Dmitry Lomov (no reviews)
We forgot once case. Marking "not lgtm" to prevent accidental landing. https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): ...
6 years ago (2014-12-10 23:02:47 UTC) #14
caitp (gmail)
https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/100001/src/runtime/runtime-array.cc#newcode467 src/runtime/runtime-array.cc:467: On 2014/12/10 23:02:47, Dmitry Lomov (chromium) wrote: > Oops, ...
6 years ago (2014-12-11 00:35:59 UTC) #15
Dmitry Lomov (no reviews)
One does not simple create a SLOPPY_ARGUMENTS_ELEMENT_KIND :)) (this is tricky stuff, and we overlook ...
6 years ago (2014-12-11 11:05:47 UTC) #16
caitp (gmail)
https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/array-concat.js File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/array-concat.js#newcode164 test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); On ...
6 years ago (2014-12-11 13:09:35 UTC) #17
Dmitry Lomov (no reviews)
Some comments. Also I realized a bigger issue with this CL - it never goes ...
6 years ago (2014-12-11 14:01:50 UTC) #18
caitp (gmail)
https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/array-concat.js File test/mjsunit/harmony/array-concat.js (right): https://codereview.chromium.org/771483002/diff/110001/test/mjsunit/harmony/array-concat.js#newcode164 test/mjsunit/harmony/array-concat.js:164: var args = (function() { return arguments; })(1,2,3); On ...
6 years ago (2014-12-11 14:41:17 UTC) #19
Dmitry Lomov (no reviews)
https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-array.cc#newcode583 src/runtime/runtime-array.cc:583: IterateExternalArrayElements<FixedInt8Array, int8_t>( Please rename IterateExternalArrayElements into IterateTypedArrayElements (this no ...
6 years ago (2014-12-11 15:11:30 UTC) #20
caitp (gmail)
https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-array.cc File src/runtime/runtime-array.cc (right): https://codereview.chromium.org/771483002/diff/170001/src/runtime/runtime-array.cc#newcode583 src/runtime/runtime-array.cc:583: IterateExternalArrayElements<FixedInt8Array, int8_t>( On 2014/12/11 15:11:29, Dmitry Lomov (chromium) wrote: ...
6 years ago (2014-12-11 16:33:09 UTC) #21
Dmitry Lomov (no reviews)
lgtm
6 years ago (2014-12-11 16:53:49 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/771483002/190001
6 years ago (2014-12-12 18:11:59 UTC) #24
commit-bot: I haz the power
6 years ago (2014-12-12 18:38:47 UTC) #25
Message was sent while issue was closed.
Committed patchset #11 (id:190001)

Powered by Google App Engine
This is Rietveld 408576698