Array Builtin Refactoring: Creating API methods on ElementsAccessor

Array Builtin Refactoring: Creating API methods on ElementsAccessor

BUG=

Committed: https://crrev.com/ffb3a927592ecc4dc6858ca8fb0d9a0c31e3fb53
Cr-Commit-Position: refs/heads/master@{#29958}

[Camillo Bruni, 2015-07-30 10:33:14]

cbruni@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2015-07-30 11:54:59]

https://codereview.chromium.org/1260283002/diff/140001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1260283002/diff/140001/src/builtins.cc#newcod...
src/builtins.cc:332: int new_length = accessor->Push(array, elms_obj, &args[0],
1, to_add,
If you pass in &args[1] here, you don't need the |start| parameter, right?

I'm not too happy about breaking the abstraction provided by "args" here
(regarding order and whatnot). But I don't see a good alternative.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1223: Handle<JSArray> array = Handle<JSArray>::cast(receiver);
Just take a Handle<JSArray> (instead of Handle<JSObject>) as a parameter.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1224: int len = Smi::cast(array->length())->value();
Let's use the opportunity to clean up the types here. Use uint32_t for array
lengths, element counts, and capacities.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1231: DCHECK(static_cast<int>(push_size) <= (Smi::kMaxValue -
len));
This should cast the other way round: 
DCHECK(push_size <= static_cast<uint32_t>(Smi::kMaxValue) - len);

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1238: new_elms =
receiver->GetIsolate()->factory()->NewUninitializedFixedArray(
This should use ElementsAccessorBase::ConvertElementsWithCapacity to do the
heavy lifting.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1255: if (direction == -1) {
s/-1/kDirectionReverse/

Or consider this:

STATIC_ASSERT(kDirectionForward == 1);
STATIC_ASSERT(kDirectionBackward == -1);
Object* object = objects[direction * index];

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1261: Handle<FixedArray>::cast(new_elms)->set(index + len,
object, mode);
This should use ElementsAccessorBase::SetImpl to abstract away the elements
kind.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1263: if (*new_elms != array->elements()) {
if (!new_elms.is_identical_to(backing_store)) {

If you want, you can guard that with a DCHECK(array->elements() ==
*backing_store) to prevent API abuse.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.cc#newcod...
src/elements.cc:1367: static uint32_t PushImpl(Handle<JSObject> receiver,
This is not needed at all if you move the other implementation up into
FastElementsAccessor.

https://codereview.chromium.org/1260283002/diff/140001/src/elements.h
File src/elements.h (right):

https://codereview.chromium.org/1260283002/diff/140001/src/elements.h#newcode133
src/elements.h:133: inline uint32_t Push(Handle<JSObject> object, Object**
objects,
I'd leave that out until it's actually needed.

[Camillo Bruni, 2015-07-31 11:48:17]

PTAL

[Jakob Kummerow, 2015-07-31 12:24:14]

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#newcod...
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#newcod...
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-fun...
File test/mjsunit/array-functions-prototype-misc.js (right):

https://codereview.chromium.org/1260283002/diff/180001/test/mjsunit/array-fun...
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-fun...
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-fun...
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.)

[Camillo Bruni, 2015-07-31 15:42:58]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-07-31 15:42:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1260283002/#ps220001
(title: "removing unused method")

[commit-bot: I haz the power, 2015-07-31 15:43:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260283002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260283002/220001

[commit-bot: I haz the power, 2015-07-31 16:10:42]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-07-31 16:11:04]

Patchset 12 (id:??) landed as
https://crrev.com/ffb3a927592ecc4dc6858ca8fb0d9a0c31e3fb53
Cr-Commit-Position: refs/heads/master@{#29958}