LGTM with a few comments below.
http://codereview.chromium.org/8356039/diff/1/src/ic.cc File src/ic.cc (left): http://codereview.chromium.org/8356039/diff/1/src/ic.cc#oldcode1845 src/ic.cc:1845: HandleScope scope(isolate()); Strange that there was a handle scope here before. That seems kind of random. Oh well.... http://codereview.chromium.org/8356039/diff/1/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1739 src/ic.cc:1739: HandleScope scope(isolate()); No need for this handle scope. http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1741 src/ic.cc:1741: if (result.is_null()) return Failure::Exception(); RETURN_IF_EMPTY_HANDLE(isolate(), result); http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1742 src/ic.cc:1742: return *value; It's a little strange not to return *result, but I guess we don't want to change the existing behavior if it makes a difference. I also guess the only reason we can't just have this whole thing be return receiver->SetElement(index, *value, strict_mode) is that there is some magic about ExternalArrayElements in the handles.cc version of SetElement. I wonder why we don't need that generally in SetElement? http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1768 src/ic.cc:1768: Handle<JSObject> receiver = Handle<JSObject>::cast(object); The existing code is confusing. We've already checked that object->IsJSObject() and we had an early return in that case (and we haven't assigned to object). So the check here is unnecessary. We already have a handle named receiver, that is Handle<JSObject>::cast(object), so it's unnecessary. http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1854 src/ic.cc:1854: ASSERT(!code.is_null()); I think operator* has the same assert for handles, so it would be caught if you tried to use it (but it's OK to have it here, too). http://codereview.chromium.org/8356039/diff/1/src/ic.cc#newcode1868 src/ic.cc:1868: TraceIC("KeyedStoreIC", name, state, target()); Make you change these sites so that TraceIC ==> TRACE_IC. http://codereview.chromium.org/8356039/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
