LGTM with comments.
The comments for ia32 apply to the other architectures as well -- I don't
think
we need AbortIfNotFixedArray() at all.
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc#newcode2548
src/ia32/macro-assembler-ia32.cc:2548:
AbortIfNotFixedArray(descriptors);
This check is pretty pointless, don't you think? You only get here if
you either just passed the fixed_array_map() check, or after loading
empty_descriptor_array() (which we can probably trust to be a
FixedArray; if you think we can't, an
ASSERT(isolate->factory()->empty_descriptor_array()->IsFixedArray() at
crankshafting time would be quite sufficient).
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc#newcode2919
src/ia32/macro-assembler-ia32.cc:2919: AbortIfNotFixedArray(edx);
This check is entirely pointless. We just did a map check on edx.
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/mark-compact.cc
File src/mark-compact.cc (right):
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/mark-compact.cc#newcode1837
src/mark-compact.cc:1837: ASSERT(descriptor_array->IsMap() ||
descriptor_array->IsUndefined());
When descriptor_array->IsMap(), then it's the back pointer, right? How
about marking that (as the comment above says)?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3420
src/objects-inl.h:3420: ASSERT(!value->IsFailure());
Can this happen?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3421
src/objects-inl.h:3421: Object* object = READ_FIELD(this,
kInstanceDescriptorsOrBackPointerOffset);
Looks like you can move this into the "else" block below.
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3442
src/objects-inl.h:3442: int Map::bit_field3() {
SMI_ACCESSORS(Map, bit_field3, kBitField3Offset)
should take care of both this and the setter below.
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3501
src/objects-inl.h:3501: // descriptor array that will contain an element
transition.
nit: s/element/elements/
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3609
src/objects-inl.h:3609: ASSERT(!value->IsFailure());
Can this happen?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3613
src/objects-inl.h:3613: } else {
What if (object->IsUndefined()) ?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h
File src/objects.h (right):
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h#newcode4617
src/objects.h:4617: // TODO(1399): It should be possible to make room
for bit_field3 in the map
outdated comment?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects.h#newcode4980
src/objects.h:4980: // TODO(1399): It should be possible to make room
for bit_field3 in the map
outdated comment?
https://chromiumcodereview.appspot.com/10692130/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
