First round of comments.
http://codereview.chromium.org/11377132/diff/1/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/11377132/diff/1/src/arm/stub-cache-arm.cc#newcode1682 src/arm/stub-cache-arm.cc:1682: nit: why the double space? http://codereview.chromium.org/11377132/diff/1/src/builtins.cc File src/builtins.cc (right): http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode328 src/builtins.cc:328: static void MoveDoubleElements(FixedDoubleArray* dst, Seems like you should be using the CopyElement routine in the appropriate ElementsAccessor from elements.cc, or at least calling CopyDoubleToDoubleElements in elements.cc. http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode640 src/builtins.cc:640: FillWithHoles(new_elms, len + index, new_length); 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. http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode722 src/builtins.cc:722: Object* first = NULL; Use ElementsAccessor::Get method here. It does what you need. http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode841 src/builtins.cc:841: if (array->HasFastSmiOrObjectElements() || array->HasFastDoubleElements()) { Might be worth adding HasFastElements, or calling IsFastElementsKind(array->elements_kind()) http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode862 src/builtins.cc:862: if (object->HasFastSmiOrObjectElements() || Might be worth adding HasFastElements, or calling IsFastElementsKind(array->elements_kind()) http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode876 src/builtins.cc:876: if (object->HasFastSmiOrObjectElements()) { 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. http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1067 src/builtins.cc:1067: if (IsFastDoubleElementsKind(elements_kind)) { Use ElementsAccessor for this, it handles the difference cases of the elements_kind http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1208 src/builtins.cc:1208: JSArray::cast(arg)->HasFastDoubleElements()) || HasFastElements http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1247 src/builtins.cc:1247: if (IsFastSmiElementsKind(elements_kind)) { You should be able to use ElementsAccessors for all the copying below. http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1315 src/builtins.cc:1315: if (array->elements()->IsFixedDoubleArray()) { Use ElementsAccessors here, too. http://codereview.chromium.org/11377132/diff/1/src/elements.cc File src/elements.cc (right): http://codereview.chromium.org/11377132/diff/1/src/elements.cc#newcode156 src/elements.cc:156: (copy_size + static_cast<int>(from_start)) <= from->length()); Please handle the hole here and handle the ElementsAccessor::kCopyToEnd and the ElementsAccessor::kCopyToEndAndInitializeToHole constants properly. http://codereview.chromium.org/11377132/diff/1/src/elements.cc#newcode183 src/elements.cc:183: if (raw_copy_size == ElementsAccessor::kCopyToEndAndInitializeToHole) { If you take my suggestion from the header file, this code (and similar like it later) will be moved out of #ifdef debug and made conditional by the FILL/DONT_FILL enum passed in. in the DONT_FILL case, the assert can remain. http://codereview.chromium.org/11377132/diff/1/src/elements.h File src/elements.h (right): http://codereview.chromium.org/11377132/diff/1/src/elements.h#newcode115 src/elements.h:115: static const int kCopyToEndAndInitializeToHole = -2; You probably should get rid of this contant above, and instead thread through a boolean value/enum {FILL_TO_END_WITH_HOLE, DONT_FILL_TO_END_WITH_HOLE} that is passed as an additional parameter to CopyElements. This would allow your call sites of CopyXXX that have to post-fill to not have to do so. http://codereview.chromium.org/11377132/diff/1/src/elements.h#newcode187 src/elements.h:187: MaybeObject* CopyDoubleToObjectElements(FixedDoubleArray* from_obj, Do you really need to add these here? Or can you get away with ElementsAccessors everywhere? http://codereview.chromium.org/11377132/diff/1/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/11377132/diff/1/src/heap.cc#newcode4184 src/heap.cc:4184: // ASSERT(JSObject::cast(obj)->HasFastSmiOrObjectElements()); Remove //? http://codereview.chromium.org/11377132/diff/1/src/heap.cc#newcode4249 src/heap.cc:4249: if (elements_kind == FAST_DOUBLE_ELEMENTS || IsFastDoubleElements(elements_kind) http://codereview.chromium.org/11377132/diff/1/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/11377132/diff/1/src/objects-inl.h#newcode1947 src/objects-inl.h:1947: Object** FixedArrayBase::data_start() { Whoa. Either return a void* and the caller has to cast appropriately, or this needs to stay a FixedArray-specific method. http://codereview.chromium.org/11377132/diff/1/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/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 Remove all the code from this line (beginning of comment) http://codereview.chromium.org/11377132/diff/1/src/parser.cc#newcode3796 src/parser.cc:3796: } ... to here. http://codereview.chromium.org/11377132/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/11377132/diff/1/src/runtime.cc#newcode4106 src/runtime.cc:4106: !IsFastSmiOrObjectElementsKind(elements_kind)) { Can't you reduce the entire multi-part test above to IsFastDoubleelementsKind(element_kind)? http://codereview.chromium.org/11377132/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
