Addressed comments.
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); On 2012/07/10 12:26:25, Jakob wrote:
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).
Done. https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc#newcode2548 src/ia32/macro-assembler-ia32.cc:2548: AbortIfNotFixedArray(descriptors); On 2012/07/10 12:26:25, Jakob wrote:
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).
Done. https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc#newcode2919 src/ia32/macro-assembler-ia32.cc:2919: AbortIfNotFixedArray(edx); On 2012/07/10 12:26:25, Jakob wrote:
This check is entirely pointless. We just did a map check on edx.
Done. https://chromiumcodereview.appspot.com/10692130/diff/8001/src/ia32/macro-assembler-ia32.cc#newcode2919 src/ia32/macro-assembler-ia32.cc:2919: AbortIfNotFixedArray(edx); On 2012/07/10 12:26:25, Jakob wrote:
This check is entirely pointless. We just did a map check on edx.
Done. 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()); It's already marked above, by accessing map->GetBackPointer(). On 2012/07/10 12:26:25, Jakob wrote:
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()); Removed. On 2012/07/10 12:26:25, Jakob wrote:
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); On 2012/07/10 12:26:25, Jakob wrote:
Looks like you can move this into the "else" block below.
Done. https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3442 src/objects-inl.h:3442: int Map::bit_field3() { On 2012/07/10 12:26:25, Jakob wrote:
SMI_ACCESSORS(Map, bit_field3, kBitField3Offset) should take care of both this and the setter below.
Done. 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. On 2012/07/10 12:26:25, Jakob wrote:
nit: s/element/elements/
Done. https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3609 src/objects-inl.h:3609: ASSERT(!value->IsFailure()); Removed. On 2012/07/10 12:26:25, Jakob wrote:
Can this happen?
https://chromiumcodereview.appspot.com/10692130/diff/8001/src/objects-inl.h#newcode3613 src/objects-inl.h:3613: } else { Then we just overwrite it, since it's the default back-pointer value. On 2012/07/10 12:26:25, Jakob wrote:
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 On 2012/07/10 12:26:25, Jakob wrote:
outdated comment?
Done. 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 On 2012/07/10 12:26:25, Jakob wrote:
outdated comment?
Done. https://chromiumcodereview.appspot.com/10692130/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
