LGTM.
Do we believe current test coverage is enough? Maybe throw in some more
tests?
http://codereview.chromium.org/3144002/diff/1/2
File src/builtins.cc (right):
http://codereview.chromium.org/3144002/diff/1/2#newcode355
src/builtins.cc:355: if (elms->map() == Heap::fixed_array_map()) return
FixedArray::cast(elms);
I don't think you need FixedArray::cast anymore.
http://codereview.chromium.org/3144002/diff/1/2#newcode363
src/builtins.cc:363: static bool IsJSArrayElementMovingAllowed(JSArray*
receiver) {
you might want to keep Fast in the name---this check allows to use
memmove/memcopy to move elements around.
http://codereview.chromium.org/3144002/diff/1/4
File src/heap.cc (right):
http://codereview.chromium.org/3144002/diff/1/4#newcode1533
src/heap.cc:1533: obj = AllocateMap(FIXED_ARRAY_TYPE,
FixedArray::kHeaderSize);
maybe move to allocation of fixed_array_map?
http://codereview.chromium.org/3144002/diff/1/6
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/6#newcode9484
src/ia32/codegen-ia32.cc:9484: // dictionary.
this comment probably needs an update. Overall, is there a way to check
other places?
http://codereview.chromium.org/3144002/diff/1/8
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/8#newcode980
src/ia32/ic-ia32.cc:980: // Check that the object is in fast mode (not
dictionary).
again fast writable mode?
http://codereview.chromium.org/3144002/diff/1/8#newcode1036
src/ia32/ic-ia32.cc:1036: // array. Check that the array is in fast
mode; if it is the
again fast writable mode? or we should just make COW an exception?
http://codereview.chromium.org/3144002/diff/1/8#newcode1886
src/ia32/ic-ia32.cc:1886: __ CmpObjectType(scratch, FIXED_ARRAY_TYPE,
scratch);
just to double check: that should be fine as StoreIC would take care of
COWs, correct?
http://codereview.chromium.org/3144002/diff/1/11
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/11#newcode1398
src/ia32/stub-cache-ia32.cc:1398: // Check that the elements are in fast
mode (not dictionary).
you may want to update the comment to mention fast writable mode.
http://codereview.chromium.org/3144002/diff/1/11#newcode1540
src/ia32/stub-cache-ia32.cc:1540: // Check that the elements are in fast
mode (not dictionary).
Ditto for comment
http://codereview.chromium.org/3144002/diff/1/14
File src/objects.cc (right):
http://codereview.chromium.org/3144002/diff/1/14#newcode5670
src/objects.cc:5670: Object* obj =
SetFastElementsCapacityAndLength(new_capacity, value);
that would apparently keep COWness of original array, was it intended?
http://codereview.chromium.org/3144002/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
