LGTM. My concern is that the names AccessorPair and AccessorInfo are too
similar and it's not obvious why the difference. We should probably try to
come
up with a better name for AccessorInfo---they are the accessors defined
through
the API, and the getter/setter are C++ functions. We might also change the
type
of getter and setter in AccessorInfo from Object to Foreign to make it more
obvious.
http://codereview.chromium.org/9152001/diff/1001/src/factory.h
File src/factory.h (right):
http://codereview.chromium.org/9152001/diff/1001/src/factory.h#newcode72
src/factory.h:72: Handle<AccessorPair> NewAccessorPair();
Maybe a comment here and also in heap.h about why these are always
pretenured (I'm not sure why they are).
I'm also not sure why we pretenure all structs. It's not obviously
best.
http://codereview.chromium.org/9152001/diff/1001/src/heap.h
File src/heap.h (right):
http://codereview.chromium.org/9152001/diff/1001/src/heap.h#newcode619
src/heap.h:619: MaybeObject* AllocateAccessorPair();
MUST_USE_RESULT to warn if a developer does not check for allocation
failure.
http://codereview.chromium.org/9152001/diff/1001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/9152001/diff/1001/src/objects.cc#newcode4611
src/objects.cc:4611: if (!maybe_accessors->To<Object>(&accessors))
return maybe_accessors;
Huh. Maybe we should get rid of MaybeObject::ToObject since it is the
same implementation (minus the Object::cast) as MaybeObject::To<Object>.
Or else continue using ToObject.
http://codereview.chromium.org/9152001/diff/1001/src/objects.cc#newcode4736
src/objects.cc:4736: AccessorPair *accessors =
AccessorPair::cast(element);
Misplaced *.
http://codereview.chromium.org/9152001/diff/1001/src/objects.cc#newcode4754
src/objects.cc:4754: AccessorPair *accessors = AccessorPair::cast(obj);
Misplaced *.
http://codereview.chromium.org/9152001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
