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.
https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode132
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#newcode143
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#newcode147
src/harmony-array.js:147: var usingIterator = ToIterable(items);
Nit: rename to iterable?
https://codereview.chromium.org/363833006/diff/290001/src/harmony-array.js#newcode148
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#newcode161
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#newcode166
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#newcode178
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/array-from.js
File test/mjsunit/harmony/array-from.js (right):
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js#newcode6
test/mjsunit/harmony/array-from.js:6: (function() {
Nit: insert empty line before.
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js#newcode36
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/array-from.js#newcode46
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/array-from.js#newcode53
test/mjsunit/harmony/array-from.js:53: 'foo': 'bar'
Same here
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js#newcode61
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/array-from.js#newcode65
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/array-from.js#newcode69
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/array-from.js#newcode78
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/array-from.js#newcode111
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/array-from.js#newcode115
test/mjsunit/harmony/array-from.js:115: function* generator() {
This seems unused here.
https://codereview.chromium.org/363833006/diff/290001/test/mjsunit/harmony/array-from.js#newcode121
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/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
