Addressed comments. I left out the enum for holes for now.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/arm/stub-cache-arm.cc#newcode1682
src/arm/stub-cache-arm.cc:1682:
On 2012/11/13 22:05:12, danno wrote:
nit: why the double space?
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc
File src/builtins.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode328
src/builtins.cc:328: static void MoveDoubleElements(FixedDoubleArray*
dst,
On 2012/11/13 22:05:12, danno wrote:
Seems like you should be using the CopyElement routine in the
appropriate
ElementsAccessor from elements.cc, or at least calling
CopyDoubleToDoubleElements in elements.cc.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode640
src/builtins.cc:640: FillWithHoles(new_elms, len + index, new_length);
On 2012/11/13 22:05:12, danno wrote:
Why does this need to be here? Can't you just do it in the case that
needs to
grow the array? You know that there will be holes in the destination
otherwise.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode722
src/builtins.cc:722: Object* first = NULL;
On 2012/11/13 22:05:12, danno wrote:
Use ElementsAccessor::Get method here. It does what you need.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode841
src/builtins.cc:841: if (array->HasFastSmiOrObjectElements() ||
array->HasFastDoubleElements()) {
On 2012/11/13 22:05:12, danno wrote:
Might be worth adding HasFastElements, or calling
IsFastElementsKind(array->elements_kind())
I originally wrote the code with int32 arrays in mind; but probably I
should add an ASSERT instead to avoid runtime overhead.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode862
src/builtins.cc:862: if (object->HasFastSmiOrObjectElements() ||
On 2012/11/13 22:05:12, danno wrote:
Might be worth adding HasFastElements, or calling
IsFastElementsKind(array->elements_kind())
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode876
src/builtins.cc:876: if (object->HasFastSmiOrObjectElements()) {
On 2012/11/13 22:05:12, danno wrote:
I think you can handle this with ElementAccessors a little more
elegantly,
ElementsAccessor* accessor = object->GetElementsAccessor();
for (.....) {
if (!accessor->HasElement(i) ...
}
dont know if its fast enough, tho.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode1067
src/builtins.cc:1067: if (IsFastDoubleElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
Use ElementsAccessor for this, it handles the difference cases of the
elements_kind
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode1208
src/builtins.cc:1208: JSArray::cast(arg)->HasFastDoubleElements()) ||
On 2012/11/13 22:05:12, danno wrote:
HasFastElements
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode1247
src/builtins.cc:1247: if (IsFastSmiElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
You should be able to use ElementsAccessors for all the copying below.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcode1315
src/builtins.cc:1315: if (array->elements()->IsFixedDoubleArray()) {
On 2012/11/13 22:05:12, danno wrote:
Use ElementsAccessors here, too.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.cc
File src/elements.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.cc#newcode156
src/elements.cc:156: (copy_size + static_cast<int>(from_start)) <=
from->length());
Removed this function again now that I just use accessors.
On 2012/11/13 22:05:12, danno wrote:
Please handle the hole here and handle the
ElementsAccessor::kCopyToEnd and the
ElementsAccessor::kCopyToEndAndInitializeToHole constants properly.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.h
File src/elements.h (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.h#newcode187
src/elements.h:187: MaybeObject*
CopyDoubleToObjectElements(FixedDoubleArray* from_obj,
On 2012/11/13 22:05:12, danno wrote:
Do you really need to add these here? Or can you get away with
ElementsAccessors
everywhere?
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/heap.cc
File src/heap.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/heap.cc#newcode4184
src/heap.cc:4184: //
ASSERT(JSObject::cast(obj)->HasFastSmiOrObjectElements());
On 2012/11/13 22:05:12, danno wrote:
Remove //?
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/objects-inl.h#newcode1947
src/objects-inl.h:1947: Object** FixedArrayBase::data_start() {
On 2012/11/13 22:05:12, danno wrote:
Whoa. Either return a void* and the caller has to cast appropriately,
or this
needs to stay a FixedArray-specific method.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc
File src/parser.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc#newcode3789
src/parser.cc:3789: // Very small array literals that don't have a
concrete hint about their type
On 2012/11/13 22:05:12, danno wrote:
Remove all the code from this line (beginning of comment)
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc#newcode3796
src/parser.cc:3796: }
On 2012/11/13 22:05:12, danno wrote:
... to here.
Done.
https://chromiumcodereview.appspot.com/11377132/diff/1/src/runtime.cc
File src/runtime.cc (right):
https://chromiumcodereview.appspot.com/11377132/diff/1/src/runtime.cc#newcode4106
src/runtime.cc:4106: !IsFastSmiOrObjectElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
Can't you reduce the entire multi-part test above to
IsFastDoubleelementsKind(element_kind)?
Done. Added ASSERT to easily find the place back when adding more raw
elementskinds.
https://chromiumcodereview.appspot.com/11377132/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
