LGTM with some minor nits...
http://codereview.chromium.org/10784014/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4368 src/objects.cc:4368: MaybeObject* maybe_accessors = CreateAccessorPairFor(name); Using a local scope makes it very clear that maybe_accessors is only used for testing/conversion immediately afterwards. It is a common v8 idiom (a.k.a. Maybe monad ;-), so I would prefer to leave the scope as it is. http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4442 src/objects.cc:4442: MaybeObject* maybe_ok = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0); See above. http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4449 src/objects.cc:4449: MaybeObject* maybe_new_map = map()->CopyDropDescriptors(); See above. http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4461 src/objects.cc:4461: maybe_ok = SetNormalizedProperty(name, structure, details); See above. http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4615 src/objects.cc:4615: MaybeObject* maybe_accessors; Scope again... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode4962 src/objects.cc:4962: // Attention, tricky index manipulation ahead: Two consecutive indices are Does this comment still hold? http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode5760 src/objects.cc:5760: MaybeObject* maybe_result = descriptor->KeyToSymbol(); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode5772 src/objects.cc:5772: MaybeObject* maybe_result = descriptor->KeyToSymbol(); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode5781 src/objects.cc:5781: MaybeObject* maybe_descriptors = Allocate(new_size, MAY_BE_SHARED); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode5814 src/objects.cc:5814: MaybeObject* maybe_result = Allocate(number_of_descriptors, shared_mode); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode5924 src/objects.cc:5924: MaybeObject* maybe_copy = heap->AllocateAccessorPair(); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode12564 src/objects.cc:12564: MaybeObject* maybe_key = heap->LookupSymbol(String::cast(k)); Scope... http://codereview.chromium.org/10784014/diff/1/src/objects.cc#newcode12607 src/objects.cc:12607: MaybeObject* maybe_new_map = obj->map()->CopyReplaceDescriptors(descriptors); Scope... http://codereview.chromium.org/10784014/diff/1/src/property.h File src/property.h (right): http://codereview.chromium.org/10784014/diff/1/src/property.h#newcode363 src/property.h:363: return GetValue(); Can we put an ASSERT here about the lookup_type_? http://codereview.chromium.org/10784014/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
