Fixed up. Thanks for the details on handles and handle scopes.
I'll wait for an OK before I actually commit this. Martin http://codereview.chromium.org/6523052/diff/15001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode509 src/ic.cc:509: Handle<Object> result; On 2011/02/18 07:22:48, Mads Ager wrote:
I would move the object_result out here (maybe call it result and call
the
handle result_handle.) Then only create the HandleScope after the
GetProperty
and ToObject part.
Then you can add a comment that you wrap the result in a handle
because
ReceiverToObjectIfRequired can cause a GC.
It would be nice to just use a handlified version of GetProperty but
that would
redo work that you already did, so I would probably leave the raw
pointers stuff
in here.
Done. http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode522 src/ic.cc:522: if (lookup.type() == INTERCEPTOR) { On 2011/02/18 07:22:48, Mads Ager wrote:
It looks like you can move the wrapping below this exception throwing
step as
well. If you throw an exception from here, there is no reason to wrap
the
receiver. However, the more important part is that LookupResults are
not GC
safe. I believe it is safe for the things you are using here (not
accessing any
heap object pointers stored on the LookupResult). In general you have
to be
careful with LookupResults. To avoid having to think about it I would
move the
wrapping to after the final checks on lookup.
Done. http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode533 src/ic.cc:533: ASSERT(*result != Heap::the_hole_value()); On 2011/02/18 07:22:48, Mads Ager wrote:
ASSERT(!result->IsTheHole());
Done. http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode805 src/ic.cc:805: Handle<Object> result; On 2011/02/18 07:22:48, Mads Ager wrote:
This case can be rewritten to just use handles. You don't need any of
the
maybe_object stuff below and you can just call the handlified version
of
GetObjectProperty (defined in handles.[h,cc]:
Handle<Object> result = GetObjectProperty(object, key);
Done. http://codereview.chromium.org/6523052/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
