Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionImplement 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 #
Messages
Total messages: 51 (3 generated)
The tricky thing with this one is that it really depends on a multitude of the harmony flags in V8, since it relies on symbols, and properly testing iterators really depends on iteration / collections. This isn't fully spec-compliant due to the lack of Realms, and should probably be more robustly tested. I've picked a few test cases from Mozilla's implementation, but they have piles and piles of test cases for this, so there is more to pull there. Anyways, I was bored and felt like extending ES6 support in V8 a bit, have a look or something.
Thanks, this is useful. Since this depends on symbols, can you add DEFINE_implication(harmony_arrays, harmony_symbols) to flag-definitions.h? 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#new... src/harmony-array.js:127: function ArrayFrom(arrayLike /* [, mapfn [, receiver] ] */) { // length == 1 Just keep the mapfn and receiver here. You can set the function length in HarmonyArrayExtendArrayPrototype. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:167: array = []; The ordering here is not correct. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:167: array = []; This should use new this if this is a constructor. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:175: } Let putStatus be Put(A, "length", k, true). is missing. That is important when this is called on something other than an array. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:177: var len = +items.length; How about adding TO_LENGTH to macros.py? https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:179: len = +0; len = 0 https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:182: array = new $Array(len); new this here as well. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:191: Put length here as well. https://codereview.chromium.org/363833006/diff/20001/test/mjsunit/harmony/arr... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-from.js:5: // Flags: --harmony Use more specific flag(s). https://codereview.chromium.org/363833006/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-from.js:13: assertArrayEquals(Array.from({length: 1, '0': { 'foo': 'bar' }}), [{'foo': 'bar'}]); Can you add test with length = -1 I wish we could also test length = Infinity, but even at 2^53-1 it would take too long. https://codereview.chromium.org/363833006/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-from.js:34: var a = ['a', 'b', 'c']; function* generator() { yield 'a'; yield 'b'; yield 'c'; } https://codereview.chromium.org/363833006/diff/20001/test/mjsunit/harmony/arr... test/mjsunit/harmony/array-from.js:52: assertThrows('Array.from([], null)', TypeError); This needs more tests. Especially things like calling Array.from.call(func, [0, 1, 2]) where the returned instance should not be an array. Also, need to test that we put length (use a setter) Also, things like checking the order of things as well as the number of times we read certain properties are important.
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#new... src/harmony-array.js:155: var value; var stepping = mapping && DEBUG_IS_ACTIVE && %DebugCallbackSupportsStepping(mapfn); https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:172: value = %_CallFunction(receiver, value, k, mapfn); if (stepping) %DebugPrepareStepInIfStepping(mapfn); https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:186: value = %_CallFunction(receiver, value, k, mapfn); ditto
Maybe we want to add stepping support later, and then also include test cases
Thanks for having a look guys, I'm fixing this up, but I had a few questions 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#new... src/harmony-array.js:127: function ArrayFrom(arrayLike /* [, mapfn [, receiver] ] */) { // length == 1 On 2014/07/02 18:26:43, arv wrote: > Just keep the mapfn and receiver here. You can set the function length in > HarmonyArrayExtendArrayPrototype. I haven't found a way to set the arity of a function in the tree, do you have an example? All of the variations on DefineProperty or SetProperty won't seem to do it. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:167: array = []; On 2014/07/02 18:26:43, arv wrote: > The ordering here is not correct. You mean, because the array is created after conditionally throwing? I felt that this might be less wasteful in cases where it does throw, but alright https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:177: var len = +items.length; On 2014/07/02 18:26:43, arv wrote: > How about adding TO_LENGTH to macros.py? Will do, but I'm not actually sure this is really correct, I think some of the other harmony array routines are just using ToInteger(array.length); https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:182: array = new $Array(len); On 2014/07/02 18:26:43, arv wrote: > new this here as well. > This needs more tests. Especially things like calling Array.from.call(func, [0, > 1, 2]) where the returned instance should not be an array. After reading that, I think I understand what you mean. But please clarify just in case I'm not getting it. If that is what you mean, per the spec, we do this (`Let A be the result of calling the [[Construct]] internal method of C with an argument list containing the single item len.`) only on the condition `IsConstructor(C)`. I'm looking for a way to figure out if `this` actually _is_ a constructor, would `IS_FUNCTION(this)` be good enough for that?
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#new... src/harmony-array.js:127: function ArrayFrom(arrayLike /* [, mapfn [, receiver] ] */) { // length == 1 On 2014/07/02 19:22:05, caitp wrote: > On 2014/07/02 18:26:43, arv wrote: > > Just keep the mapfn and receiver here. You can set the function length in > > HarmonyArrayExtendArrayPrototype. > > I haven't found a way to set the arity of a function in the tree, do you have an > example? All of the variations on DefineProperty or SetProperty won't seem to do > it. %FunctionSetLength(f, len); https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:167: array = []; On 2014/07/02 19:22:05, caitp wrote: > On 2014/07/02 18:26:43, arv wrote: > > The ordering here is not correct. > > You mean, because the array is created after conditionally throwing? I felt that > this might be less wasteful in cases where it does throw, but alright We could try to change the spec. But for now, we should just match it and track any future changes. https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... src/harmony-array.js:182: array = new $Array(len); On 2014/07/02 19:22:05, caitp wrote: > On 2014/07/02 18:26:43, arv wrote: > > new this here as well. > > > This needs more tests. Especially things like calling Array.from.call(func, > [0, > > 1, 2]) where the returned instance should not be an array. > > After reading that, I think I understand what you mean. But please clarify just > in case I'm not getting it. > > If that is what you mean, per the spec, we do this (`Let A be the result of > calling the [[Construct]] internal method of C with an argument list containing > the single item len.`) only on the condition `IsConstructor(C)`. > > I'm looking for a way to figure out if `this` actually _is_ a constructor, would > `IS_FUNCTION(this)` be good enough for that? Unfortunately V8 has no concept of [[Constructor]] but a good approximation for now is IS_FUNCTION (and maybe check for a prototype?)
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#new... src/harmony-array.js:182: array = new $Array(len); On 2014/07/02 19:29:09, arv wrote: > On 2014/07/02 19:22:05, caitp wrote: > > On 2014/07/02 18:26:43, arv wrote: > > > new this here as well. > > > > > This needs more tests. Especially things like calling Array.from.call(func, > > [0, > > > 1, 2]) where the returned instance should not be an array. > > > > After reading that, I think I understand what you mean. But please clarify > just > > in case I'm not getting it. > > > > If that is what you mean, per the spec, we do this (`Let A be the result of > > calling the [[Construct]] internal method of C with an argument list > containing > > the single item len.`) only on the condition `IsConstructor(C)`. > > > > I'm looking for a way to figure out if `this` actually _is_ a constructor, > would > > `IS_FUNCTION(this)` be good enough for that? > > Unfortunately V8 has no concept of [[Constructor]] but a good approximation for > now is IS_FUNCTION (and maybe check for a prototype?) IS_SPEC_FUNCTION is more what you want.
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#new... > src/harmony-array.js:182: array = new $Array(len); > On 2014/07/02 19:29:09, arv wrote: > > On 2014/07/02 19:22:05, caitp wrote: > > > On 2014/07/02 18:26:43, arv wrote: > > > > new this here as well. > > > > > > > This needs more tests. Especially things like calling > Array.from.call(func, > > > [0, > > > > 1, 2]) where the returned instance should not be an array. > > > > > > After reading that, I think I understand what you mean. But please clarify > > just > > > in case I'm not getting it. > > > > > > If that is what you mean, per the spec, we do this (`Let A be the result of > > > calling the [[Construct]] internal method of C with an argument list > > containing > > > the single item len.`) only on the condition `IsConstructor(C)`. > > > > > > I'm looking for a way to figure out if `this` actually _is_ a constructor, > > would > > > `IS_FUNCTION(this)` be good enough for that? > > > > Unfortunately V8 has no concept of [[Constructor]] but a good approximation > for > > now is IS_FUNCTION (and maybe check for a prototype?) > > IS_SPEC_FUNCTION is more what you want. https://codereview.chromium.org/364853009/ implements a proper IsConstructor, so if that lands first it might be better to rely on that rather than IS_SPEC_FUNCTION. However I'm not aware of any case where we'd have a Function-class object which is not a constructor.
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 > > File src/harmony-array.js (right): > > > > > https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... > > src/harmony-array.js:182: array = new $Array(len); > > On 2014/07/02 19:29:09, arv wrote: > > > On 2014/07/02 19:22:05, caitp wrote: > > > > On 2014/07/02 18:26:43, arv wrote: > > > > > new this here as well. > > > > > > > > > This needs more tests. Especially things like calling > > Array.from.call(func, > > > > [0, > > > > > 1, 2]) where the returned instance should not be an array. > > > > > > > > After reading that, I think I understand what you mean. But please clarify > > > just > > > > in case I'm not getting it. > > > > > > > > If that is what you mean, per the spec, we do this (`Let A be the result > of > > > > calling the [[Construct]] internal method of C with an argument list > > > containing > > > > the single item len.`) only on the condition `IsConstructor(C)`. > > > > > > > > I'm looking for a way to figure out if `this` actually _is_ a constructor, > > > would > > > > `IS_FUNCTION(this)` be good enough for that? > > > > > > Unfortunately V8 has no concept of [[Constructor]] but a good approximation > > for > > > now is IS_FUNCTION (and maybe check for a prototype?) > > > > IS_SPEC_FUNCTION is more what you want. > > https://codereview.chromium.org/364853009/ implements a proper IsConstructor, so > if that lands first it might be better to rely on that rather than > IS_SPEC_FUNCTION. However I'm not aware of any case where we'd have a > Function-class object which is not a constructor. Those cases are primarily natives, and functions created with .bind. But there currently is no easy way to check for them consistently (the predicate in the mentioned CL is incorrect, unfortunately).
On 2014/07/08 10:48:43, rossberg wrote: > 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 > > > File src/harmony-array.js (right): > > > > > > > > > https://codereview.chromium.org/363833006/diff/20001/src/harmony-array.js#new... > > > src/harmony-array.js:182: array = new $Array(len); > > > On 2014/07/02 19:29:09, arv wrote: > > > > On 2014/07/02 19:22:05, caitp wrote: > > > > > On 2014/07/02 18:26:43, arv wrote: > > > > > > new this here as well. > > > > > > > > > > > This needs more tests. Especially things like calling > > > Array.from.call(func, > > > > > [0, > > > > > > 1, 2]) where the returned instance should not be an array. > > > > > > > > > > After reading that, I think I understand what you mean. But please > clarify > > > > just > > > > > in case I'm not getting it. > > > > > > > > > > If that is what you mean, per the spec, we do this (`Let A be the result > > of > > > > > calling the [[Construct]] internal method of C with an argument list > > > > containing > > > > > the single item len.`) only on the condition `IsConstructor(C)`. > > > > > > > > > > I'm looking for a way to figure out if `this` actually _is_ a > constructor, > > > > would > > > > > `IS_FUNCTION(this)` be good enough for that? > > > > > > > > Unfortunately V8 has no concept of [[Constructor]] but a good > approximation > > > for > > > > now is IS_FUNCTION (and maybe check for a prototype?) > > > > > > IS_SPEC_FUNCTION is more what you want. > > > > https://codereview.chromium.org/364853009/ implements a proper IsConstructor, > so > > if that lands first it might be better to rely on that rather than > > IS_SPEC_FUNCTION. However I'm not aware of any case where we'd have a > > Function-class object which is not a constructor. > > Those cases are primarily natives, and functions created with .bind. But there > currently is no easy way to check for them consistently (the predicate in the > mentioned CL is incorrect, unfortunately). Ah I see. I noticed the comments about the other CL, I can take a stab at implementing a proper `%IsConstructor()` but maybe that should be another CL. `IS_SPEC_FUNCTION()` should be good enough for the time being, I think. Apart from that, more tests could be ported in from spidermonkey's jit-runtime tests, but otherwise I think it's about there.
PTAL: FIXME added, will try to implement IsConstructor in separate CL
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#ne... src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration Why does this depend in harmony_iteration? https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#ne... src/harmony-array.js:142: if (IS_NULL_OR_UNDEFINED(receiver)) { Why is null included here? I thought this was part of bullet point 5 but I don't see how this relates to the spec?
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#ne... src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration On 2014/07/16 20:31:48, arv wrote: > Why does this depend in harmony_iteration? There are two cases in the Array.from spec: 1) iterables, and 2) array-like objects. The dependency on iterators is important for things like strings especially, because treating strings as array-like objects causes problems with surrogate pairs, while iterating over string characters doesn't. (without this flag, one of the tests from mozilla verifying that this works correctly for Array.from(string) will fail) https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#ne... src/harmony-array.js:142: if (IS_NULL_OR_UNDEFINED(receiver)) { On 2014/07/16 20:31:48, arv wrote: > Why is null included here? I thought this was part of bullet point 5 but I don't > see how this relates to the spec? The pattern was just taken from one of the ES5 routines, but yes, it should be undefined or callable or throw --- I'll fix that up.
So, I was thinking about implementing CreateDataProperty* for this, but as far as I'm aware, simply assigning properties to the result object should have the same effect, and it makes the code a bit harder to read if it's written this way. Happy to do it, but I thought I'd hold off on that.
On 2014/07/19 19:52:14, caitp wrote: > So, I was thinking about implementing CreateDataProperty* for this, but as far > as I'm aware, simply assigning properties to the result object should have the > same effect, and it makes the code a bit harder to read if it's written this > way. > > Happy to do it, but I thought I'd hold off on that. Not sure what this is replying to, but in general, assignment is quite different from creating a property e.g. with Object.defineProperty. The only case where you can be sure that assignment actually creates a property is if the prototype of the object is null. Otherwise, there might be setters or read-only data properties up the prototype chain that could intercept.
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#ne... src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and harmony_iteration On 2014/07/19 18:29:06, caitp wrote: > On 2014/07/16 20:31:48, arv wrote: > > Why does this depend in harmony_iteration? > > There are two cases in the Array.from spec: 1) iterables, and 2) array-like > objects. > > The dependency on iterators is important for things like strings especially, > because treating strings as array-like objects causes problems with surrogate > pairs, while iterating over string characters doesn't. > > (without this flag, one of the tests from mozilla verifying that this works > correctly for Array.from(string) will fail) I can see why the test depends on harmony_iteration but this code does not. Also, if it did you would have needed to add a DEFINE_IMPLICATION in flag-definitions.h. So, just update the comment and all is good.
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#ne... > src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and > harmony_iteration > On 2014/07/19 18:29:06, caitp wrote: > > On 2014/07/16 20:31:48, arv wrote: > > > Why does this depend in harmony_iteration? > > > > There are two cases in the Array.from spec: 1) iterables, and 2) array-like > > objects. > > > > The dependency on iterators is important for things like strings especially, > > because treating strings as array-like objects causes problems with surrogate > > pairs, while iterating over string characters doesn't. > > > > (without this flag, one of the tests from mozilla verifying that this works > > correctly for Array.from(string) will fail) > > I can see why the test depends on harmony_iteration but this code does not. > Also, if it did you would have needed to add a DEFINE_IMPLICATION in > flag-definitions.h. > > So, just update the comment and all is good. Okay --- I would personally prefer to define the flag implication, because I think the behaviour is actually just wrong/only half correct without iterators, but depends what you guys think
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 > > File src/harmony-array.js (right): > > > > https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#ne... > > src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and > > harmony_iteration > > On 2014/07/19 18:29:06, caitp wrote: > > > On 2014/07/16 20:31:48, arv wrote: > > > > Why does this depend in harmony_iteration? > > > > > > There are two cases in the Array.from spec: 1) iterables, and 2) array-like > > > objects. > > > > > > The dependency on iterators is important for things like strings especially, > > > because treating strings as array-like objects causes problems with surrogate > > > pairs, while iterating over string characters doesn't. > > > > > > (without this flag, one of the tests from mozilla verifying that this works > > > correctly for Array.from(string) will fail) > > > > I can see why the test depends on harmony_iteration but this code does not. > > Also, if it did you would have needed to add a DEFINE_IMPLICATION in > > flag-definitions.h. > > > > So, just update the comment and all is good. > > Okay --- I would personally prefer to define the flag implication, because I think the behaviour is actually just wrong/only half correct without iterators, but depends what you guys think I think that is a valid concern. It would be bad if code started to depend on the wrong behavior for strings. I therefore support splitting this into a new file and having it depend on harmony_iteration too. Andreas, WDYT?
On 2014/07/22 14:13:12, arv wrote: > 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 > > > File src/harmony-array.js (right): > > > > > > > https://codereview.chromium.org/363833006/diff/150001/src/harmony-array.js#ne... > > > src/harmony-array.js:126: // ES6, 22.1.2.1 --- Requires harmony_symbols and > > > harmony_iteration > > > On 2014/07/19 18:29:06, caitp wrote: > > > > On 2014/07/16 20:31:48, arv wrote: > > > > > Why does this depend in harmony_iteration? > > > > > > > > There are two cases in the Array.from spec: 1) iterables, and 2) > array-like > > > > objects. > > > > > > > > The dependency on iterators is important for things like strings > especially, > > > > because treating strings as array-like objects causes problems with > surrogate > > > > pairs, while iterating over string characters doesn't. > > > > > > > > (without this flag, one of the tests from mozilla verifying that this > works > > > > correctly for Array.from(string) will fail) > > > > > > I can see why the test depends on harmony_iteration but this code does not. > > > Also, if it did you would have needed to add a DEFINE_IMPLICATION in > > > flag-definitions.h. > > > > > > So, just update the comment and all is good. > > > > Okay --- I would personally prefer to define the flag implication, because I > think the behaviour is actually just wrong/only half correct without iterators, > but depends what you guys think > > I think that is a valid concern. It would be bad if code started to depend on > the wrong behavior for strings. I therefore support splitting this into a new > file and having it depend on harmony_iteration too. > > Andreas, WDYT? I'm fine either way. The cleanest way would be to split this off into a separate file gated by both flags. But given that we plan to ship iterators soon anyway that's perhaps over-engineering it, and just causes more work later (I'm assuming that the odd behaviour for strings will never make it into an official Chrome version either way).
Okay, maybe just leave it for now unless iterators gets pushed back a milestone
PTAL --- there is an issue now, the generators test fails with the current bleeding edge, was passing previously (as far as I can tell, this shouldn't be caused by the changes to the ArrayFrom implementation, so I think something broke elsewhere, or maybe the test is no longer valid)
https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:22: // assertArrayLikeEquals(Array.from.call(thisArg, generator()), ['a', 'b', 'c'], constructor); It looks like one of the flags added for mjsunit tests is breaking this: It breaks with the following configuration: out/x64.release/d8 --random-seed=454874028 --turbo-filter=* --always-opt --nohard-abort --nodead-code-elimination --nofold-constants --harmony-arrays --harmony-generators V8 version 3.29.13 (candidate) [console: dumb] d8> (function() { function* generator() { yield '1'; yield '2'; yield '3'; } return Array.from(generator()); })() # # Fatal error in , line 0 # unimplemented code # ==== C stack trace =============================== 1: ?? 2: ?? 3: ?? But if you just pass --harmony-arrays --harmony-generators, it works as expected
On 2014/08/21 16:56:34, caitp wrote: > https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/ar... > File test/mjsunit/harmony/array-from.js (right): > > https://codereview.chromium.org/363833006/diff/230001/test/mjsunit/harmony/ar... > test/mjsunit/harmony/array-from.js:22: // > assertArrayLikeEquals(Array.from.call(thisArg, generator()), ['a', 'b', 'c'], > constructor); > It looks like one of the flags added for mjsunit tests is breaking this: > > It breaks with the following configuration: > > out/x64.release/d8 --random-seed=454874028 --turbo-filter=* --always-opt > --nohard-abort --nodead-code-elimination --nofold-constants --harmony-arrays > --harmony-generators > V8 version 3.29.13 (candidate) [console: dumb] > d8> (function() { function* generator() { yield '1'; yield '2'; yield '3'; } > return Array.from(generator()); })() > > > # > # Fatal error in , line 0 > # unimplemented code > # > > ==== C stack trace =============================== > > 1: ?? > 2: ?? > 3: ?? > > But if you just pass --harmony-arrays --harmony-generators, it works as expected Probably related to TurboFan. I see other generator tests being disabled: https://codereview.chromium.org/342453002/diff/70008/test/mjsunit/mjsunit.status
bug filed for that issue, since it affects bleeding_edge without this patch applied https://code.google.com/p/v8/issues/detail?id=3527
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
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#ne... src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { Per spec, Array.from's length should be 1. See // length == N pattern elsewhere.
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#ne... src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { On 2014/09/05 04:04:42, jsbell wrote: > Per spec, Array.from's length should be 1. See // length == N pattern elsewhere. See line 204, it was suggested I do it that way rather than make the function signature harder to read
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#ne... src/harmony-array.js:127: function ArrayFrom(arrayLike, mapfn, receiver) { On 2014/09/05 13:13:41, caitp wrote: > On 2014/09/05 04:04:42, jsbell wrote: > > Per spec, Array.from's length should be 1. See // length == N pattern > elsewhere. > > See line 204, it was suggested I do it that way rather than make the function > signature harder to read Acknowledged.
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#ne... src/harmony-array.js:126: // ES6, draft 07-18-14, section 22.1.2.1 08-24 is the latest atm https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... src/harmony-array.js:154: A = IS_SPEC_FUNCTION(C) ? new C() : []; extra whitespace here https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... src/harmony-array.js:173: A = IS_SPEC_FUNCTION(C) ? new C() : []; This should be: A = IS_SPEC_FUNCTION(C) ? new C(len) : new $Array(len); Please add a test for this case too. https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from-generators.js (right): https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from-generators.js:22: function* generator() { Move this up, before the first usage. https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:11: assertArrayLikeEquals(Array.from.call(thisArg, [], undefined), [], constructor); long line here and elsewhere
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#ne... > src/harmony-array.js:126: // ES6, draft 07-18-14, section 22.1.2.1 > 08-24 is the latest atm > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... > src/harmony-array.js:154: A = IS_SPEC_FUNCTION(C) ? new C() : []; > extra whitespace here > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... > src/harmony-array.js:173: A = IS_SPEC_FUNCTION(C) ? new C() : []; > This should be: > > A = IS_SPEC_FUNCTION(C) ? new C(len) : new $Array(len); > > Please add a test for this case too. > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > File test/mjsunit/harmony/array-from-generators.js (right): > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > test/mjsunit/harmony/array-from-generators.js:22: function* generator() { > Move this up, before the first usage. > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > File test/mjsunit/harmony/array-from.js (right): > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > test/mjsunit/harmony/array-from.js:11: > assertArrayLikeEquals(Array.from.call(thisArg, [], undefined), [], constructor); > long line here and elsewhere Thanks, updated... I'm still a bit unsure if the semantics for calling `mapfn` are supposed to be identical to the ES262 methods which behave similarly (the code was originally copied from ArrayMap). It looks like the spec is pretty specific about "If thisArg was supplied, let T be thisArg; else let T be undefined.". Is it just because the rest of the logic lives in [[Call]] rather than in these algorithms, or is it rather that harmony is supposed to ignore strict mode vs sloppy mode etc
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 > > File src/harmony-array.js (right): > > > > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... > > src/harmony-array.js:126: // ES6, draft 07-18-14, section 22.1.2.1 > > 08-24 is the latest atm > > > > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... > > src/harmony-array.js:154: A = IS_SPEC_FUNCTION(C) ? new C() : []; > > extra whitespace here > > > > > https://codereview.chromium.org/363833006/diff/250001/src/harmony-array.js#ne... > > src/harmony-array.js:173: A = IS_SPEC_FUNCTION(C) ? new C() : []; > > This should be: > > > > A = IS_SPEC_FUNCTION(C) ? new C(len) : new $Array(len); > > > > Please add a test for this case too. > > > > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > > File test/mjsunit/harmony/array-from-generators.js (right): > > > > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > > test/mjsunit/harmony/array-from-generators.js:22: function* generator() { > > Move this up, before the first usage. > > > > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > > File test/mjsunit/harmony/array-from.js (right): > > > > > https://codereview.chromium.org/363833006/diff/250001/test/mjsunit/harmony/ar... > > test/mjsunit/harmony/array-from.js:11: > > assertArrayLikeEquals(Array.from.call(thisArg, [], undefined), [], > constructor); > > long line here and elsewhere > > Thanks, updated... I'm still a bit unsure if the semantics for calling `mapfn` > are supposed to be identical to the ES262 methods which behave similarly (the > code was originally copied from ArrayMap). It looks like the spec is pretty > specific about "If thisArg was supplied, let T be thisArg; else let T be > undefined.". Is it just because the rest of the logic lives in [[Call]] rather > than in these algorithms, or is it rather that harmony is supposed to ignore > strict mode vs sloppy mode etc I believe the spec describes what should happen with [[Call]] of a sloppy function when passed undefined/null and that the code you have is how we need to do it in V8 when we use %_CallFunction.
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#ne... src/harmony-array.js:141: receiver = %GetDefaultReceiver(mapfn) || undefined; Can you add tests for this too?
Add tests for calling mapfn with/without receiver in/out of sloppy mode
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#ne... src/harmony-array.js:128: var C = this; Nit: drop this redundant variable. https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:132: if (IS_UNDEFINED(mapfn)) { var mapping = !IS_UNDEFINED(mapfn) But in fact, it's redundant to define this variable at all. https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:143: receiver = ToObject(receiver); Hm, is this actually necessary? Doesn't _CallFunction do it already? https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:147: var usingIterator = ToIterable(items); Nit: rename to iterable? https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:148: var k; These can all be locally declared. Also, please use more descriptive names, even if the spec doesn't (A => result). https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:161: while (!(next = iterator.next()).done) { I think for (var next = iterator.next(); !next.done; next = iterator.next()) would be much clearer. In fact, we shipped iterators, so you should be able to say just for (var next of iterable) and remove much of the boilerplate here... unless I'm missing something. https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:166: mappedValue = mapping ? %_CallFunction(receiver, nextValue, k, mapfn) : nextValue; Nit: line length https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:178: mappedValue = mapping ? %_CallFunction(receiver, nextValue, k, mapfn) : nextValue; Nit: line length https://codereview.chromium.org/363833006/diff/290001/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/363833006/diff/290001/src/runtime.js#newcode469 src/runtime.js:469: function TO_LENGTH(arg) { This should be ToLength, and move to the next section, e.g. next to ToInteger. https://codereview.chromium.org/363833006/diff/290001/src/runtime.js#newcode471 src/runtime.js:471: if (arg < +0) { Nit: The + is redundant (and may potentially be more expensive). https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:6: (function() { Nit: insert empty line before. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:36: constructor); Nit: style guide requires indent of 4 for line continuations (also below). https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:46: 'foo': 'bar' Nit: write this on one line https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:53: 'foo': 'bar' Same here https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:61: var kSet = (new Set).add('foo').add('bar').add('baz'); The Set constructor can take an array argument. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:65: var kMap = (new Map).set(0, 'foo').set(1, 'bar').set(2, 'baz'); Same here. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:69: // TODO: re-enable generator test (failing with "Fatal error unimplemented code") Nit: line lengths https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:78: assertArrayLikeEquals(Array.from.call(thisArg, gclef + " G"), [gclef, " ", "G"], Line length https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:111: testArrayFrom(Other, Other); Cover more cases here, e.g. a function with @@iterator, a function that is not a constructor. Also test what happens on primitive constructors, esp String. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:115: function* generator() { This seems unused here. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:121: function assertArrayLikeEquals(value, expected, type) { Move this to the start of the file.
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#ne... src/harmony-array.js:161: while (!(next = iterator.next()).done) { On 2014/09/10 07:23:55, rossberg wrote: > I think > > for (var next = iterator.next(); !next.done; next = iterator.next()) > > would be much clearer. In fact, we shipped iterators, so you should be able to > say just > > for (var next of iterable) > > and remove much of the boilerplate here... unless I'm missing something. Using for-of is going to be an observable difference due to IteratorClose which checks and calls .return in case of an abrubt completion
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#ne... > src/harmony-array.js:161: while (!(next = iterator.next()).done) { > On 2014/09/10 07:23:55, rossberg wrote: > > I think > > > > for (var next = iterator.next(); !next.done; next = iterator.next()) > > > > would be much clearer. In fact, we shipped iterators, so you should be able to > > say just > > > > for (var next of iterable) > > > > and remove much of the boilerplate here... unless I'm missing something. > > Using for-of is going to be an observable difference due to IteratorClose which > checks and calls .return in case of an abrubt completion Oh dear, that thing... OTOH, shouldn't internal uses of iterators do the same then? (I seem to remember that we discussed this, but don't remember the arguments.)
Thanks for the review, I've addressed a number of nits and will push a new patchset shortly. 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#ne... src/harmony-array.js:143: receiver = ToObject(receiver); On 2014/09/10 07:23:55, rossberg wrote: > Hm, is this actually necessary? Doesn't _CallFunction do it already? Looks like Execution::Call() does this, yes --- edit, no, apparently this does not do the right thing, so we do actually need to set this up in the script. (For sloppy-mode, Execution::Call is not passing the correct global variable when null/undefined is used, as it looks like it should --- this causes test failures). https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... src/harmony-array.js:148: var k; On 2014/09/10 07:23:55, rossberg wrote: > These can all be locally declared. Also, please use more descriptive names, even > if the spec doesn't (A => result). More descriptive name is good --- I think declaring these outside of the blocks which use them is good in this case --- it's redundant to define them locally for both blocks, and confusing if they're defined in one local block and the second block depends on the hoisted variable. https://codereview.chromium.org/363833006/diff/290001/src/runtime.js File src/runtime.js (right): https://codereview.chromium.org/363833006/diff/290001/src/runtime.js#newcode469 src/runtime.js:469: function TO_LENGTH(arg) { On 2014/09/10 07:23:55, rossberg wrote: > This should be ToLength, and move to the next section, e.g. next to ToInteger. As you saw, this was moved into a separate CL so that dpino@ could make use of it, I think it would be better to land crrev.com/552273002 and remove the definition from this CL entirely (which would result to changing it to ToLength()) https://codereview.chromium.org/363833006/diff/290001/src/runtime.js#newcode471 src/runtime.js:471: if (arg < +0) { On 2014/09/10 07:23:55, rossberg wrote: > Nit: The + is redundant (and may potentially be more expensive). Removed in crrev.com/552273002 https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:111: testArrayFrom(Other, Other); On 2014/09/10 07:23:55, rossberg wrote: > Cover more cases here, e.g. a function with @@iterator, a function that is not a > constructor. Also test what happens on primitive constructors, esp String. Can you provide examples of functions which we would not consider constructors? I think .bind() is supposed to behave this way, as well as arrow functions --- but I'm not sure if they actually _do_ behave this way currently.
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:111: testArrayFrom(Other, Other); On 2014/09/10 14:26:05, caitp wrote: > On 2014/09/10 07:23:55, rossberg wrote: > > Cover more cases here, e.g. a function with @@iterator, a function that is not > a > > constructor. Also test what happens on primitive constructors, esp String. > > Can you provide examples of functions which we would not consider constructors? > I think .bind() is supposed to behave this way, as well as arrow functions --- > but I'm not sure if they actually _do_ behave this way currently. Yes, a test with .bind would be good. Also, most ES library functions are not constructors.
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#ne... src/harmony-array.js:143: receiver = ToObject(receiver); On 2014/09/10 14:26:04, caitp wrote: > On 2014/09/10 07:23:55, rossberg wrote: > > Hm, is this actually necessary? Doesn't _CallFunction do it already? > > Looks like Execution::Call() does this, yes --- edit, no, apparently this does > not do the right thing, so we do actually need to set this up in the script. > (For sloppy-mode, Execution::Call is not passing the correct global variable > when null/undefined is used, as it looks like it should --- this causes test > failures). Yes. We don't want _CallFunction to have to do the extra work to do this in general. That is why it is up to all the callers to handle it. We could provide a helper function that does this, which would clean up the repeated code we already have. https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... File test/mjsunit/harmony/array-from.js (right): https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/ar... test/mjsunit/harmony/array-from.js:111: testArrayFrom(Other, Other); On 2014/09/10 14:32:35, rossberg wrote: > On 2014/09/10 14:26:05, caitp wrote: > > On 2014/09/10 07:23:55, rossberg wrote: > > > Cover more cases here, e.g. a function with @@iterator, a function that is > not > > a > > > constructor. Also test what happens on primitive constructors, esp String. > > > > Can you provide examples of functions which we would not consider > constructors? > > I think .bind() is supposed to behave this way, as well as arrow functions --- > > but I'm not sure if they actually _do_ behave this way currently. > > Yes, a test with .bind would be good. Also, most ES library functions are not > constructors. Anything that is lacking a .prototype. For example Math.cos or Array.prototype.filter for example.
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 > > File src/harmony-array.js (right): > > > > > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... > > src/harmony-array.js:161: while (!(next = iterator.next()).done) { > > On 2014/09/10 07:23:55, rossberg wrote: > > > I think > > > > > > for (var next = iterator.next(); !next.done; next = iterator.next()) > > > > > > would be much clearer. In fact, we shipped iterators, so you should be able > to > > > say just > > > > > > for (var next of iterable) > > > > > > and remove much of the boilerplate here... unless I'm missing something. > > > > Using for-of is going to be an observable difference due to IteratorClose > which > > checks and calls .return in case of an abrubt completion > > Oh dear, that thing... > > OTOH, shouldn't internal uses of iterators do the same then? (I seem to remember > that we discussed this, but don't remember the arguments.) I just posted this to es-discuss
Patchset #17 (id:310001) has been deleted
On 2014/09/10 14:46:37, arv wrote: > 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 > > > File src/harmony-array.js (right): > > > > > > > > > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... > > > src/harmony-array.js:161: while (!(next = iterator.next()).done) { > > > On 2014/09/10 07:23:55, rossberg wrote: > > > > I think > > > > > > > > for (var next = iterator.next(); !next.done; next = iterator.next()) > > > > > > > > would be much clearer. In fact, we shipped iterators, so you should be > able > > to > > > > say just > > > > > > > > for (var next of iterable) > > > > > > > > and remove much of the boilerplate here... unless I'm missing something. > > > > > > Using for-of is going to be an observable difference due to IteratorClose > > which > > > checks and calls .return in case of an abrubt completion > > > > Oh dear, that thing... > > > > OTOH, shouldn't internal uses of iterators do the same then? (I seem to > remember > > that we discussed this, but don't remember the arguments.) > > I just posted this to es-discuss So where do we stand on the explicit iteration vs for..of thing? Can we switch back to for..of? (I've updated this CL for the first time in months now).
On 2014/12/03 00:37:21, caitp wrote: > On 2014/09/10 14:46:37, arv wrote: > > 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 > > > > File src/harmony-array.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#ne... > > > > src/harmony-array.js:161: while (!(next = iterator.next()).done) { > > > > On 2014/09/10 07:23:55, rossberg wrote: > > > > > I think > > > > > > > > > > for (var next = iterator.next(); !next.done; next = iterator.next()) > > > > > > > > > > would be much clearer. In fact, we shipped iterators, so you should be > > able > > > to > > > > > say just > > > > > > > > > > for (var next of iterable) > > > > > > > > > > and remove much of the boilerplate here... unless I'm missing something. > > > > > > > > Using for-of is going to be an observable difference due to IteratorClose > > > which > > > > checks and calls .return in case of an abrubt completion > > > > > > Oh dear, that thing... > > > > > > OTOH, shouldn't internal uses of iterators do the same then? (I seem to > > remember > > > that we discussed this, but don't remember the arguments.) > > > > I just posted this to es-discuss > > So where do we stand on the explicit iteration vs for..of thing? Can we switch > back to for..of? > > (I've updated this CL for the first time in months now). There essentially was agreement that this was a bug in the spec and that internal uses of iterators should like for-of, but nobody has done the work of filing a bug identifying all relevant places, and consequently, I don't see the agreement reflected in the draft yet. However, despite the formalities I think it's fine to assume that it's going to be fixed, and use for-of in the implementation.
LGTM We have to keep our eyes on the next spec draft since there are some changes coming here.
On 2014/12/03 19:20:29, arv wrote: > LGTM > > We have to keep our eyes on the next spec draft since there are some changes > coming here. Well it's behind an unstaged flag still, so there's a bit of time :D Would a small standard lib feature like this also need the full blown launch process with thread and everything?
On 2014/12/03 19:22:53, caitp wrote: > On 2014/12/03 19:20:29, arv wrote: > > LGTM > > > > We have to keep our eyes on the next spec draft since there are some changes > > coming here. > > Well it's behind an unstaged flag still, so there's a bit of time :D Would a > small standard lib feature like this also need the full blown launch process > with thread and everything? For individual functions that would be overkill. But I think we'll want to lump shipping all the new array functionality together, and in that case it will probably justify an accumulative intent-to-ship. But there is still stuff missing.
On 2014/12/04 11:36:00, rossberg wrote: > On 2014/12/03 19:22:53, caitp wrote: > > On 2014/12/03 19:20:29, arv wrote: > > > LGTM > > > > > > We have to keep our eyes on the next spec draft since there are some changes > > > coming here. > > > > Well it's behind an unstaged flag still, so there's a bit of time :D Would a > > small standard lib feature like this also need the full blown launch process > > with thread and everything? > > For individual functions that would be overkill. But I think we'll want to lump > shipping all the new array functionality together, and in that case it will > probably justify an accumulative intent-to-ship. But there is still stuff > missing. Okay --- it would be good to have one more green light before landing it I think, at least.
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#ne... src/harmony-array.js:152: // Step 8. Nit: perhaps remove these fine-grained references, as they are bound to get out of sync.
The CQ bit was checked by caitpotter88@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/363833006/410001
Message was sent while issue was closed.
Committed patchset #21 (id:410001) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
