First round of comments.
https://codereview.chromium.org/21079003/diff/2007/src/harmony-array.js
File src/harmony-array.js (right):
https://codereview.chromium.org/21079003/diff/2007/src/harmony-array.js#newcode48
src/harmony-array.js:48: var length = ToUint32(array.length);
The access to "array.length" is observable and needs to happen before
checking whether "predicate" is callable, let's move lines 47 and 48 up
to before the check.
Also I am not sure if that's an oversight in the spec draft or not, but
the draft currently specifies ToInteger to be used on the array length.
https://codereview.chromium.org/21079003/diff/2007/src/harmony-array.js#newcode56
src/harmony-array.js:56: if (%HasProperty(array, key)) {
Better use the "in" keyword here, that's more efficient and doesn't
require a preceding %ToName() call either.
https://codereview.chromium.org/21079003/diff/2007/src/harmony-array.js#newcode58
src/harmony-array.js:58: if (%_CallFunction(thisArg, element, i, array,
predicate)) {
When using the optimized %_CallFunction intrinsic we need to make sure
that the receiver is correct. If you take a look at some other Array
functions (e.g. ArraySome) you can see how that is done. In general you
can these functions as a good template as they have been thoroughly
tested.
https://codereview.chromium.org/21079003/diff/2007/src/harmony-array.js#newcode69
src/harmony-array.js:69: function ArrayFindIndex(predicate /* thisArg
*/) { // length == 1
Most comments from above apply to this function as well.
https://codereview.chromium.org/21079003/diff/2007/test/mjsunit/harmony/array-find.js
File test/mjsunit/harmony/array-find.js (right):
https://codereview.chromium.org/21079003/diff/2007/test/mjsunit/harmony/array-find.js#newcode281
test/mjsunit/harmony/array-find.js:281:
nit: Empty line at end of file, ./tools/presubmit.py will complain.
https://codereview.chromium.org/21079003/diff/2007/test/mjsunit/harmony/array-findindex.js
File test/mjsunit/harmony/array-findindex.js (right):
https://codereview.chromium.org/21079003/diff/2007/test/mjsunit/harmony/array-findindex.js#newcode1
test/mjsunit/harmony/array-findindex.js:1: // Copyright 2013 the V8
project authors. All rights reserved.
nit: Too much white-space.
https://codereview.chromium.org/21079003/diff/2007/test/mjsunit/harmony/array-findindex.js#newcode281
test/mjsunit/harmony/array-findindex.js:281:
nit: Empty line at end of file, ./tools/presubmit.py will complain.
https://codereview.chromium.org/21079003/
--
--
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/groups/opt_out.
