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

Issue 363833006: Implement Array.from() (Closed)

Created:
6 years, 5 months ago by caitp (gmail)
Modified:
6 years ago
CC:
v8-dev
Project:
v8
Visibility:
Public.

Description

Implement Array.from() A helpful utility which converts iterables and array-like objects into Arrays https://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.from LOG=Y BUG=v8:3336 R=arv@chromium.org, rossberg@chromium.org

Patch Set 1 : Implement Array.from() #

Patch Set 2 : Update EXPECTED_BUILTINS_COUNT #

Total comments: 23

Patch Set 3 : Refactor based on review and add additional tests #

Patch Set 4 : Rebase, remove dead code, improve constructor test #

Patch Set 5 : share abstract ToLength operation #

Patch Set 6 : Minor whitespace cleanup #

Patch Set 7 : Rebase against bleeding_edge #

Patch Set 8 : Add FIXME to implement IsConstructor correctly (using IS_SPEC_FUNCTION for now) #

Patch Set 9 : Update expected builtins #

Total comments: 5

Patch Set 10 : Don't accept null-value mapFn #

Patch Set 11 : #

Patch Set 12 : Change comment, Array.from does not __require__ iterators #

Patch Set 13 : Rebased, rewrote algorithm to closer match spec #

Total comments: 1

Patch Set 14 : Disable generators tests for TurboFan #

Total comments: 8

Patch Set 15 : Address comments, rebased #

Total comments: 1

Patch Set 16 : Add tests for calling mapfn with/without receiver in/out of sloppy mode #

Total comments: 30

Patch Set 17 : More tests, some cleanup #

Patch Set 18 : Rebased + remove unneeded pieces #

Patch Set 19 : Use For..Of loop in implementation, remove extra ToLength impl #

Total comments: 1

Patch Set 20 : Nit: remove step labels #

Patch Set 21 : Rebase it #

Unified diffs Side-by-side diffs Delta from patch set Stats (+176 lines, -0 lines) Patch
M src/harmony-array.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +53 lines, -0 lines 0 comments Download
A test/mjsunit/harmony/array-from.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +123 lines, -0 lines 0 comments Download

Messages

Total messages: 51 (3 generated)
caitp (gmail)
The tricky thing with this one is that it really depends on a multitude of ...
6 years, 5 months ago (2014-07-02 16:42:32 UTC) #1
arv (Not doing code reviews)
Thanks, this is useful. Since this depends on symbols, can you add DEFINE_implication(harmony_arrays, harmony_symbols) to ...
6 years, 5 months ago (2014-07-02 18:26:44 UTC) #2
aandrey
FYI https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#newcode155 src/harmony-array.js:155: var value; var stepping = mapping && DEBUG_IS_ACTIVE ...
6 years, 5 months ago (2014-07-02 18:28:16 UTC) #3
Yang
Maybe we want to add stepping support later, and then also include test cases
6 years, 5 months ago (2014-07-02 19:05:40 UTC) #4
caitp (gmail)
Thanks for having a look guys, I'm fixing this up, but I had a few ...
6 years, 5 months ago (2014-07-02 19:22:06 UTC) #5
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#newcode127 src/harmony-array.js:127: function ArrayFrom(arrayLike /* [, mapfn [, receiver] ] */) ...
6 years, 5 months ago (2014-07-02 19:29:09 UTC) #6
rossberg
https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#newcode182 src/harmony-array.js:182: array = new $Array(len); On 2014/07/02 19:29:09, arv wrote: ...
6 years, 5 months ago (2014-07-03 11:16:57 UTC) #7
caitp (gmail)
On 2014/07/03 11:16:57, rossberg wrote: > https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js > File src/harmony-array.js (right): > > https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#newcode182 > ...
6 years, 5 months ago (2014-07-07 20:57:07 UTC) #8
rossberg
On 2014/07/07 20:57:07, caitp wrote: > On 2014/07/03 11:16:57, rossberg wrote: > > https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js > ...
6 years, 5 months ago (2014-07-08 10:48:43 UTC) #9
caitp (gmail)
On 2014/07/08 10:48:43, rossberg wrote: > On 2014/07/07 20:57:07, caitp wrote: > > On 2014/07/03 ...
6 years, 5 months ago (2014-07-11 03:35:21 UTC) #10
caitp (gmail)
PTAL: FIXME added, will try to implement IsConstructor in separate CL
6 years, 5 months ago (2014-07-16 11:27:52 UTC) #11
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#newcode126 src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration Why ...
6 years, 5 months ago (2014-07-16 20:31:48 UTC) #12
caitp (gmail)
https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#newcode126 src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration On ...
6 years, 5 months ago (2014-07-19 18:29:06 UTC) #13
caitp (gmail)
So, I was thinking about implementing CreateDataProperty* for this, but as far as I'm aware, ...
6 years, 5 months ago (2014-07-19 19:52:14 UTC) #14
rossberg
On 2014/07/19 19:52:14, caitp wrote: > So, I was thinking about implementing CreateDataProperty* for this, ...
6 years, 5 months ago (2014-07-21 09:38:04 UTC) #15
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#newcode126 src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration On ...
6 years, 5 months ago (2014-07-21 18:00:34 UTC) #16
caitp (gmail)
On 2014/07/21 18:00:34, arv wrote: > https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js > File src/harmony-array.js (right): > > https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#newcode126 > ...
6 years, 5 months ago (2014-07-22 13:52:28 UTC) #17
arv (Not doing code reviews)
On 2014/07/22 at 13:52:28, caitpotter88 wrote: > On 2014/07/21 18:00:34, arv wrote: > > https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js ...
6 years, 5 months ago (2014-07-22 14:13:12 UTC) #18
rossberg
On 2014/07/22 14:13:12, arv wrote: > On 2014/07/22 at 13:52:28, caitpotter88 wrote: > > On ...
6 years, 5 months ago (2014-07-22 14:35:59 UTC) #19
caitp (gmail)
Okay, maybe just leave it for now unless iterators gets pushed back a milestone
6 years, 5 months ago (2014-07-22 14:40:11 UTC) #20
caitp (gmail)
PTAL --- there is an issue now, the generators test fails with the current bleeding ...
6 years, 4 months ago (2014-08-21 16:19:47 UTC) #21
caitp (gmail)
https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/array-from.js File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/array-from.js#newcode22 test/mjsunit/harmony/array-from.js:22: // assertArrayLikeEquals(Array.from.call(thisArg, generator()), ['a', 'b', 'c'], constructor); It looks ...
6 years, 4 months ago (2014-08-21 16:56:34 UTC) #22
arv (Not doing code reviews)
On 2014/08/21 16:56:34, caitp wrote: > https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/array-from.js > File test/mjsunit/harmony/array-from.js (right): > > https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/array-from.js#newcode22 > ...
6 years, 4 months ago (2014-08-21 16:59:06 UTC) #23
caitp (gmail)
bug filed for that issue, since it affects bleeding_edge without this patch applied https://code.google.com/p/v8/issues/detail?id=3527
6 years, 4 months ago (2014-08-21 17:52:56 UTC) #24
jsbell
Little nit from inspection... https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#newcode127 src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { ...
6 years, 3 months ago (2014-09-05 04:04:43 UTC) #26
caitp (gmail)
https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#newcode127 src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { On 2014/09/05 04:04:42, jsbell ...
6 years, 3 months ago (2014-09-05 13:13:41 UTC) #27
jsbell
https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#newcode127 src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { On 2014/09/05 13:13:41, caitp ...
6 years, 3 months ago (2014-09-05 15:50:34 UTC) #28
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#newcode126 src/harmony-array.js:126: // ES6, draft 07-18-14, section 22.1.2.1 08-24 is the ...
6 years, 3 months ago (2014-09-05 18:04:54 UTC) #29
caitp (gmail)
On 2014/09/05 18:04:54, arv wrote: > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js > File src/harmony-array.js (right): > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#newcode126 > ...
6 years, 3 months ago (2014-09-05 19:09:47 UTC) #30
arv (Not doing code reviews)
On 2014/09/05 19:09:47, caitp wrote: > On 2014/09/05 18:04:54, arv wrote: > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js > ...
6 years, 3 months ago (2014-09-05 19:15:32 UTC) #31
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/270001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/270001/src/harmony-array.js#newcode141 src/harmony-array.js:141: receiver = %GetDefaultReceiver(mapfn) || undefined; Can you add tests ...
6 years, 3 months ago (2014-09-05 19:17:40 UTC) #32
caitp (gmail)
Add tests for calling mapfn with/without receiver in/out of sloppy mode
6 years, 3 months ago (2014-09-05 19:46:36 UTC) #33
rossberg
https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode128 src/harmony-array.js:128: var C = this; Nit: drop this redundant variable. ...
6 years, 3 months ago (2014-09-10 07:23:56 UTC) #34
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode161 src/harmony-array.js:161: while (!(next = iterator.next()).done) { On 2014/09/10 07:23:55, rossberg ...
6 years, 3 months ago (2014-09-10 14:05:14 UTC) #35
rossberg
On 2014/09/10 14:05:14, arv wrote: > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js > File src/harmony-array.js (right): > > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode161 > ...
6 years, 3 months ago (2014-09-10 14:22:07 UTC) #36
caitp (gmail)
Thanks for the review, I've addressed a number of nits and will push a new ...
6 years, 3 months ago (2014-09-10 14:26:05 UTC) #37
rossberg
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js#newcode111 test/mjsunit/harmony/array-from.js:111: testArrayFrom(Other, Other); On 2014/09/10 14:26:05, caitp wrote: > On ...
6 years, 3 months ago (2014-09-10 14:32:36 UTC) #38
arv (Not doing code reviews)
https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode143 src/harmony-array.js:143: receiver = ToObject(receiver); On 2014/09/10 14:26:04, caitp wrote: > ...
6 years, 3 months ago (2014-09-10 14:33:58 UTC) #39
arv (Not doing code reviews)
On 2014/09/10 14:22:07, rossberg wrote: > On 2014/09/10 14:05:14, arv wrote: > > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js > ...
6 years, 3 months ago (2014-09-10 14:46:37 UTC) #40
caitp (gmail)
On 2014/09/10 14:46:37, arv wrote: > On 2014/09/10 14:22:07, rossberg wrote: > > On 2014/09/10 ...
6 years ago (2014-12-03 00:37:21 UTC) #42
rossberg
On 2014/12/03 00:37:21, caitp wrote: > On 2014/09/10 14:46:37, arv wrote: > > On 2014/09/10 ...
6 years ago (2014-12-03 14:45:34 UTC) #43
arv (Not doing code reviews)
LGTM We have to keep our eyes on the next spec draft since there are ...
6 years ago (2014-12-03 19:20:29 UTC) #44
caitp (gmail)
On 2014/12/03 19:20:29, arv wrote: > LGTM > > We have to keep our eyes ...
6 years ago (2014-12-03 19:22:53 UTC) #45
rossberg
On 2014/12/03 19:22:53, caitp wrote: > On 2014/12/03 19:20:29, arv wrote: > > LGTM > ...
6 years ago (2014-12-04 11:36:00 UTC) #46
caitp (gmail)
On 2014/12/04 11:36:00, rossberg wrote: > On 2014/12/03 19:22:53, caitp wrote: > > On 2014/12/03 ...
6 years ago (2014-12-08 15:47:33 UTC) #47
rossberg
lgtm https://codereview.chromium.org/363833006/diff/370001/src/harmony-array.js File src/harmony-array.js (right): https://codereview.chromium.org/363833006/diff/370001/src/harmony-array.js#newcode152 src/harmony-array.js:152: // Step 8. Nit: perhaps remove these fine-grained ...
6 years ago (2014-12-10 13:58:17 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/363833006/410001
6 years ago (2014-12-11 16:49:26 UTC) #50
commit-bot: I haz the power
6 years ago (2014-12-11 17:16:28 UTC) #51
Message was sent while issue was closed.
Committed patchset #21 (id:410001)

Powered by Google App Engine
This is Rietveld 408576698