Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181903003:
Allow TypedArrays to be initialized with iterables (Closed)
|
Created:
5 years, 6 months ago by Dan Ehrenberg Modified:
5 years, 6 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAllow TypedArrays to be initialized with iterables
In ES6, the TypedArray constructor can be called either with an
array-like object or an iterable. The code previously handled
only array-like objects. This patch switches to supporting
iterables while throwing in an optimization to make Arrays
get allocated the old way, without an extra copy.
BUG=v8:4090
LOG=Y
R=adamk
Committed: https://crrev.com/40420f67e7fb029a1379882f95023afaa19fa7a0
Cr-Commit-Position: refs/heads/master@{#29031}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix iterator multiple lookup issue #
Total comments: 3
Patch Set 3 : fix iteration #
Total comments: 2
Patch Set 4 : clean up how iteration works #
Total comments: 4
Patch Set 5 : null proto for iterator #
Total comments: 8
Patch Set 6 : Fixes from arv's comments #Messages
Total messages: 22 (4 generated)
https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js#newcode154 src/typedarray.js:154: // twice. Currently, that's fine, but it'll be observable with proxies. Proxies aren't required, a getter for @@iterator is sufficient to detect this. Let's fix it now.
https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js#newcode154 src/typedarray.js:154: // twice. Currently, that's fine, but it'll be observable with proxies. On 2015/06/12 00:57:08, adamk wrote: > Proxies aren't required, a getter for @@iterator is sufficient to detect this. > Let's fix it now. I think the only way to really fix that currently is to write the loop by hand :( As is done in ArrayFrom --- Is there a better way?
Fixed the issue in the new version. https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/1/src/typedarray.js#newcode154 src/typedarray.js:154: // twice. Currently, that's fine, but it'll be observable with proxies. On 2015/06/12 01:11:33, caitp wrote: > On 2015/06/12 00:57:08, adamk wrote: > > Proxies aren't required, a getter for @@iterator is sufficient to detect this. > > Let's fix it now. > > I think the only way to really fix that currently is to write the loop by hand > :( As is done in ArrayFrom --- Is there a better way? Well, you can use for-of on most of the builtin iterators (generators, array iterators, etc), as the iterator defines [Symbol.iterator]() to return this, but that's not really part of the standard iteration protocol, so I won't depend on it here. The loop wasn't so bad.
https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js#newco... src/typedarray.js:140: list.push(current.value); Sorry to nitpick, but this is missing a piece of the iteration protocol, mainly step 7.4.2.4
arv@chromium.org changed reviewers: + arv@chromium.org
https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js#newco... src/typedarray.js:139: while (!current.done) { The main benefit of using for-of is that you get everything for free. Maybe we should create a macro that does everything right and then when we add support for iterator return all these places will get it for free.
Good catch, thanks https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/20001/src/typedarray.js#newco... src/typedarray.js:140: list.push(current.value); On 2015/06/12 02:47:21, caitp wrote: > Sorry to nitpick, but this is missing a piece of the iteration protocol, mainly > step 7.4.2.4 Done.
https://codereview.chromium.org/1181903003/diff/40001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/40001/src/typedarray.js#newco... src/typedarray.js:140: if (!IS_OBJECT(current)) { I think you want IS_SPEC_OBJECT here. The proper tests for this are sadly quite large (and probably not worth adding just for this case), hope we can replace this with a macro soon.
https://codereview.chromium.org/1181903003/diff/40001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/40001/src/typedarray.js#newco... src/typedarray.js:140: if (!IS_OBJECT(current)) { On 2015/06/12 18:06:45, adamk wrote: > I think you want IS_SPEC_OBJECT here. > > The proper tests for this are sadly quite large (and probably not worth adding > just for this case), hope we can replace this with a macro soon. How's this, just using for-of with a wrapper? We can factor that pattern into a function if we want.
https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js#newco... src/typedarray.js:139: newIterable[symbolIterator] = function() { return iterator; }; Sadly this will fail if someone monkeypatches Object.prototype[Symbol.iterator]. I guess you could use %CreateDataProperty once it exists. But I'm fine with your previous code, changed to IS_SPEC_OBJECT with a TODO to make it nicer later.
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js#newco... src/typedarray.js:139: newIterable[symbolIterator] = function() { return iterator; }; On 2015/06/13 00:17:27, adamk wrote: > Sadly this will fail if someone monkeypatches Object.prototype[Symbol.iterator]. > I guess you could use %CreateDataProperty once it exists. But I'm fine with your > previous code, changed to IS_SPEC_OBJECT with a TODO to make it nicer l The object can easily have a null prototype to avoid that. This pattern could be used in ArrayFrom and maybe other places too. Maybe that's not a bad idea
https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js#newco... src/typedarray.js:139: newIterable[symbolIterator] = function() { return iterator; }; On 2015/06/13 00:49:24, caitp wrote: > On 2015/06/13 00:17:27, adamk wrote: > > Sadly this will fail if someone monkeypatches > Object.prototype[Symbol.iterator]. > > I guess you could use %CreateDataProperty once it exists. But I'm fine with > your > > previous code, changed to IS_SPEC_OBJECT with a TODO to make it nicer l > > The object can easily have a null prototype to avoid that. This pattern could be > used in ArrayFrom and maybe other places too. Maybe that's not a bad idea Ah, yeah: var newIterable = { __proto__: null }; newIterable[symbolIterator] = function() { return iterator; };
https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/60001/src/typedarray.js#newco... src/typedarray.js:139: newIterable[symbolIterator] = function() { return iterator; }; On 2015/06/13 00:50:44, adamk wrote: > On 2015/06/13 00:49:24, caitp wrote: > > On 2015/06/13 00:17:27, adamk wrote: > > > Sadly this will fail if someone monkeypatches > > Object.prototype[Symbol.iterator]. > > > I guess you could use %CreateDataProperty once it exists. But I'm fine with > > your > > > previous code, changed to IS_SPEC_OBJECT with a TODO to make it nicer l > > > > The object can easily have a null prototype to avoid that. This pattern could > be > > used in ArrayFrom and maybe other places too. Maybe that's not a bad idea > > Ah, yeah: > > var newIterable = { __proto__: null }; > newIterable[symbolIterator] = function() { return iterator; }; Oh good idea. Seemed to pass the test. I guess computed properties would solve this too, but I guess they might not be ready yet.
LGTM https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:138: var newIterable = {__proto__: null}; var newIterable = { __proto__: null, [symbolIterator]() { return iterator; } }; https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:138: var newIterable = {__proto__: null}; This hack/workaround needs a comment. https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:140: for (var value of newIterable) { var list = [...newIterable]; would work if we were OK with that dependency. https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:159: // twice. Currently, that's fine, but it'll be observable with proxies. This comment is out of date now
https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js File src/typedarray.js (right): https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:138: var newIterable = {__proto__: null}; On 2015/06/15 15:53:19, arv wrote: > This hack/workaround needs a comment. Done. https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:138: var newIterable = {__proto__: null}; On 2015/06/15 15:53:18, arv wrote: > var newIterable = { > __proto__: null, > [symbolIterator]() { return iterator; } > }; Done. https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:140: for (var value of newIterable) { On 2015/06/15 15:53:19, arv wrote: > var list = [...newIterable]; > > would work if we were OK with that dependency. Would it? I thought it was important to use an InternalArray rather than a GlobalArray. https://codereview.chromium.org/1181903003/diff/80001/src/typedarray.js#newco... src/typedarray.js:159: // twice. Currently, that's fine, but it'll be observable with proxies. On 2015/06/15 15:53:18, arv wrote: > This comment is out of date now Deleted.
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org Link to the patchset: https://codereview.chromium.org/1181903003/#ps100001 (title: "Fixes from arv's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181903003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/40420f67e7fb029a1379882f95023afaa19fa7a0 Cr-Commit-Position: refs/heads/master@{#29031} |
