A couple of suggestions to make the code clearer. Once those are addressed
we
are ready to go.
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#newcode449
src/ic.cc:449: if (object->IsString() || object->IsNumber() ||
object->IsBoolean()) {
On 2011/02/18 00:55:38, Martin Maly wrote:
I don't understand why the object was double-wrapped in a handle.
Based on my
understanding it is not necessary but if it was necessary I wonder why
the
Handle<Object> object which arrives straight from the IC. I am
assuming GC will
discover that object by walking the stack.
Yes, there is no reason to rewrap the incoming objects.
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode509
src/ic.cc:509: Handle<Object> result;
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.
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode522
src/ic.cc:522: if (lookup.type() == INTERCEPTOR) {
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.
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode533
src/ic.cc:533: ASSERT(*result != Heap::the_hole_value());
ASSERT(!result->IsTheHole());
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode541
src/ic.cc:541: Handle<JSFunction> function(JSFunction::cast(*result));
On 2011/02/18 00:55:38, Martin Maly wrote:
No HandleScope needed here, I believe ... the Handle will simply use
the one
above (line 508).
Yes, that's fine.
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode552
src/ic.cc:552: if (result->IsJSFunction()) return *result;
On 2011/02/18 00:55:38, Martin Maly wrote:
I am a bit hazy on using
"return handle.EscapeFrom(&scope)"
vs.
"return *handle;"
I saw both in the codebase...
This method returns a MaybeObject which is a raw object pointer. It can
either be a Smi, an Object pointer or a failure. The caller will deal
with failures (either throwing exceptions or garbage collecting or just
returning the result). So here, you just have to extract the raw Object*
from the handle and return that. This is done with "return *handle".
handle.EscapeFrom(&scope) is used rarely. It is used in situations where
you have a local handle scope and you want to return one of those
handles to the caller (who has his own handle scope). If you just return
the handle you will get into trouble because that handle belongs to your
local handle scope. Once the local handle scope gets destructed as part
of returning, the GC no longer knows about the value in your handle and
you are no better off than with a raw pointer. handle.EscapeFrom(&scope)
moves the handle from the current scope to the outer scope which makes
it safe to return it. The outer scope is still active after returning
and the GC will therefore update the pointer in the handle.
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode805
src/ic.cc:805: Handle<Object> result;
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);
http://codereview.chromium.org/6523052/diff/15001/src/ic.cc#newcode821
src/ic.cc:821: return TypeError("property_not_function", object, key);
On 2011/02/18 00:55:38, Martin Maly wrote:
This sequence seemed cleaner, get rid of "answer" altogether.
Yes, thanks for cleaning that up! The original code was very strange. :)
http://codereview.chromium.org/6523052/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
