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);
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
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.
Fixed.
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());
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
JSObject::cast does the same assert.
Fixed.
http://codereview.chromium.org/7385006/diff/7003/src/objects-inl.h#newcode4373
src/objects-inl.h:4373: ASSERT(other->IsJSObject());
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Ditto.
Fixed.
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
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
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.
Fixed.
http://codereview.chromium.org/7385006/diff/7003/src/objects.cc#newcode11772
src/objects.cc:11772: MaybeObject* maybe_hash = key->GetIdentityHash();
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
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.
Fixed (partially, didn't create an enum so far). I suggest turning it
into an enum in a separate CL.
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);
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Document what is returned when there are no existing hidden properties
and
create_if_needed is false.
Fixed.
http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2933
src/objects.h:2933: class ObjectDictionary: public
Dictionary<ObjectDictionaryShape, JSObject*> {
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Needs a comment.
Fixed.
http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2946
src/objects.h:2946: MUST_USE_RESULT MaybeObject* Add(JSObject* key,
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Same here.
Are PropertyDetails really required? If not, change the entry size in
the shape
to 2.
In that case the ObjectDictionary should use HashTable instead of
Dictionary as a base class, because those PropertyDetails are the only
difference (besides being enumerable) between them. Right?
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) {
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Make this a method on Factory?
Fixed.
http://codereview.chromium.org/7385006/diff/7003/test/cctest/test-dictionary.cc#newcode48
test/cctest/test-dictionary.cc:48: TEST(ObjectDictionary) {
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
Test adding more entries to the dictionary and check the returned
values.
Fixed.
http://codereview.chromium.org/7385006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
