First round of comments.
https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode1680 src/objects.cc:1680: MaybeObject* ret; Can we call this maybe_result for consistency? https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode1709 src/objects.cc:1709: if (!ret->ToObject(&obj)) return ret; if (ret->IsFailure()) return ret; I actually prefer: MaybeObject* maybe_failure = NormalizeProperties(...; if (maybe_failure->IsFailure()) return maybe_failure; since ret will be overwritten in the next line anyway. The values seem independent. https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode1720 src/objects.cc:1720: return ret; Yes. We should not jump into handlified code from non-handlified code. This is very prone to errors, especially since the caller of the function does not know that handlified code may cause GC without returning failure, potentially invalidating all raw pointers in the client code. Either we have to ensure that no code is handlified for implementing NotifyObservers, or we'll have to pull it up to the handlified client of this functionality. On 2012/10/31 14:44:31, rafaelw wrote:
Isn't this unsafe? The notifyObservers could have called GC. Don't
you need to:
if (ret->IsFailure()) return ret; Handle<Object> return_handle(return_value->ToObjectUnchecked()); // call NotifyObservers return *return_handle;
https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.cc#newcode7620 src/objects.cc:7620: (other->bit_field3() & IsObserved::kMask) && Can we just use is_observed() == other->is_observed()? https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.h File src/objects.h (right): https://chromiumcodereview.appspot.com/11347037/diff/1/src/objects.h#newcode2279 src/objects.h:2279: void NotifyObservers(const char* type, String* name, Object* oldValue); Yes, use old_value in the C++ code. Also in other files. On 2012/10/31 17:27:24, rafaelw wrote:
also, are arguments unix_hacker style?
https://chromiumcodereview.appspot.com/11347037/diff/1/src/runtime.cc File src/runtime.cc (right): https://chromiumcodereview.appspot.com/11347037/diff/1/src/runtime.cc#newcode13240 src/runtime.cc:13240: if (!maybe->To<Map>(&map)) return maybe; The <Map> is not needed; maybe->To(&map) works just fine. https://chromiumcodereview.appspot.com/11347037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
