LGTM with a few more minor comments.
https://codereview.chromium.org/1260283002/diff/180001/src/elements.cc
File src/elements.cc (right):
https://codereview.chromium.org/1260283002/diff/180001/src/elements.cc#newcode1180
src/elements.cc:1180: // to_add is > 0 and new_length <= elms_len, so
elms_obj cannot be the
nit: s/to_add/push_size/
https://codereview.chromium.org/1260283002/diff/180001/src/elements.cc#newcode1190
src/elements.cc:1190: int offset = direction * index;
This implementation depends on the direction sentinels being precisely
+1 and -1, so please guard that assumption with:
STATIC_ASSERT(ElementsAccessor::kDirectionForward == 1);
STATIC_ASSERT(ElementsAccessor::kDirectionBackward == -1);
(A STATIC_ASSERT is essentially a DCHECK that's enforced by the C++
compiler at compile time, so you can only use it for constants, but on
the up side it generates no code at all.)
https://codereview.chromium.org/1260283002/diff/180001/src/elements.h
File src/elements.h (right):
https://codereview.chromium.org/1260283002/diff/180001/src/elements.h#newcode129
src/elements.h:129: Handle<FixedArrayBase> backing_store, Object**
objects,
as discussed, maybe leave a comment here:
// TODO(cbruni): Consider passing Arguments* instead of Object**
depending on
// the requirements of future callers.
https://codereview.chromium.org/1260283002/diff/180001/src/elements.h#newcode132
src/elements.h:132: inline uint32_t Push(Handle<JSArray> receiver,
Object** objects,
This overload doesn't have any callers, does it? Why provide it when
it's not needed?
https://codereview.chromium.org/1260283002/diff/180001/test/mjsunit/array-functions-prototype-misc.js
File test/mjsunit/array-functions-prototype-misc.js (right):
https://codereview.chromium.org/1260283002/diff/180001/test/mjsunit/array-functions-prototype-misc.js#newcode357
test/mjsunit/array-functions-prototype-misc.js:357: get length() {
return 10},
nit: s/10}/10; }/
https://codereview.chromium.org/1260283002/diff/180001/test/mjsunit/array-functions-prototype-misc.js#newcode372
test/mjsunit/array-functions-prototype-misc.js:372: get length() {
return 10 },
nit: s/10/10;/
https://codereview.chromium.org/1260283002/diff/180001/test/mjsunit/array-functions-prototype-misc.js#newcode385
test/mjsunit/array-functions-prototype-misc.js:385: assertEquals(2,
Array.prototype.push.call(receiver, 'third', 'second'));
(as discussed: it's saddening that you can do this, but good to at least
have a test for it.)
https://codereview.chromium.org/1260283002/
--
--
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.
