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#newcode332
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#newcode1223
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#newcode1224
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#newcode1231
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#newcode1238
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#newcode1255
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#newcode1261
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#newcode1263
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#newcode1367
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.
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.
