http://codereview.chromium.org/7385006/diff/7003/src/handles.cc File src/handles.cc (right):
http://codereview.chromium.org/7385006/diff/7003/src/handles.cc#newcode433 src/handles.cc:433: CALL_HEAP_FUNCTION(obj->GetIsolate(), obj->GetIdentityHash(), Smi); Creating a handle for a Smi is a waste. Use something like CALL_AND_RETRY(obj->GetIsolate(), obj->GetIdentityHash(), Smi::cast(__object__)->value(), 0) and change the return type to int. http://codereview.chromium.org/7385006/diff/7003/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/7385006/diff/7003/src/objects-inl.h#newcode4360 src/objects-inl.h:4360: ASSERT(other->IsJSObject()); JSObject::cast does the same assert. http://codereview.chromium.org/7385006/diff/7003/src/objects-inl.h#newcode4373 src/objects-inl.h:4373: ASSERT(other->IsJSObject()); Ditto. http://codereview.chromium.org/7385006/diff/7003/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/7385006/diff/7003/src/objects.cc#newcode2988 src/objects.cc:2988: // in the prototype chain. Note that HasLocalProperty() can cause a GC in The comment about HasLocalProperty should be moved to where it's called in the function below. Also it'd be nice to add AssertNoAllocation there. http://codereview.chromium.org/7385006/diff/7003/src/objects.cc#newcode11772 src/objects.cc:11772: MaybeObject* maybe_hash = key->GetIdentityHash(); It'd be nice to avoid allocating a hidden properties object in this case. Looks like GetIdentityHash needs create_if_needed too. Since the flag gets more usage our style is to create an enum for it to aid readability. http://codereview.chromium.org/7385006/diff/7003/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode1641 src/objects.h:1641: MUST_USE_RESULT MaybeObject* GetHiddenProperties(bool create_if_needed); Document what is returned when there are no existing hidden properties and create_if_needed is false. http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2933 src/objects.h:2933: class ObjectDictionary: public Dictionary<ObjectDictionaryShape, JSObject*> { Needs a comment. http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2942 src/objects.h:2942: int FindEntry(Isolate* isolate, JSObject* key); Doesn't seem to be defined/called. Remove? http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2946 src/objects.h:2946: MUST_USE_RESULT MaybeObject* Add(JSObject* key, Same here. Are PropertyDetails really required? If not, change the entry size in the shape to 2. http://codereview.chromium.org/7385006/diff/7003/test/cctest/test-dictionary.cc File test/cctest/test-dictionary.cc (right): http://codereview.chromium.org/7385006/diff/7003/test/cctest/test-dictionary.cc#newcode41 test/cctest/test-dictionary.cc:41: static Handle<ObjectDictionary> NewObjectDictionary(int at_least_space_for) { Make this a method on Factory? http://codereview.chromium.org/7385006/diff/7003/test/cctest/test-dictionary.cc#newcode48 test/cctest/test-dictionary.cc:48: TEST(ObjectDictionary) { Test adding more entries to the dictionary and check the returned values. http://codereview.chromium.org/7385006/diff/7003/test/cctest/test-dictionary.cc#newcode66 test/cctest/test-dictionary.cc:66: Handle<JSObject> o = FACTORY->NewJSArray(100); We need larger arrays here to exhaust the new space. There's a flag max_new_space_size that can help, but I'm not sure how to use it in a cctest. http://codereview.chromium.org/7385006/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
