I took a quick look. I think there are two places that are GC hazards, mentioned below.
http://codereview.chromium.org/7799026/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode140 src/objects.cc:140: if (IsSmi()) { You can use IsNumber() here and remove the clause for IsHeapNumber() below. http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode231 src/objects.cc:231: Object** args[] = { receiver.location(), name.location() }; We seem to have a mix of this style and Handle<Object> args[] = { receiver, name }; Handle<Object> result = CallTrap(..., HandleVector(args, ARRAY_SIZE(args)); I prefer the Handle version, partly because mentioning Handle<T>::location() indicates something special and weird going on and so I prefer to keep it rare. Also, seeing Handle in a signature makes it obvious (?) that the function is in the handle world and should not be called from the raw pointer world. http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1882 src/objects.cc:1882: Object* value) { Indentation is now off. http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1910 src/objects.cc:1910: result->HandlerResult(JSProxy::cast(pt)); I like to write return result->HandlerResult(JSProxy::cast(pt)); http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2295 src/objects.cc:2295: JSReceiver* receiver_raw, String* name_raw) { Also one parameter per line here. http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2329 src/objects.cc:2329: const char* name, Object* derived, int argc, Object*** args) { I think there's a GC hazard here. derived is a raw pointer, and looking up a symbol in the symbol table can cause allocation and thus GC (it may have to allocate the symbol and may also have to grow the symbol table). Change derived to Handle<Object> in the signature? http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2353 src/objects.cc:2353: MaybeObject* JSObject::SetPropertyForResult(LookupResult* result, I think SetPropertyForResult needs a better name but I'm at a loss for ideas. http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2377 src/objects.cc:2377: JSObject* object = JSObject::cast(this); Isn't this already JSObject*? http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2411 src/objects.cc:2411: Handle<Object> result = proxy->CallTrap( There's also a GC hazard here. CallTrap can silently perform GC and there's no way for the caller of this function to find that out. http://codereview.chromium.org/7799026/diff/1/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7799026/diff/1/src/objects.h#newcode6613 src/objects.h:6613: int argc, Object*** args); We consistently use one line per parameter in declarations that don't fit on a single line. Even when they're related like here. http://codereview.chromium.org/7799026/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/7799026/diff/1/src/runtime.cc#newcode4496 src/runtime.cc:4496: if (result) return isolate->heap()->true_value(); You can also write this without the if as: return isolate->heap()->ToBoolean(result); http://codereview.chromium.org/7799026/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
