Since Slava is on vacation I thought it would be great if someone else could
review this. Is it OK to pass this on to you Mads?
http://codereview.chromium.org/7385006/
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
I'd be happy to review this, but I don't understand how it's supposed to
work
when a hash allocation fails on lookup? E.g., if I add this code to the
provided
test
for (int i = 0; i < 10; i++) {
Handle o = FACTORY->NewJSArray(100);
dict->FindEntry(*o);
}
it triggers the asser
I guess the intent was to rely on the fact that objects without allocated
hash
codes can't possibly be already present as keys in a dictionary. In this
case
the lookup logic should probably use a dummy hash code.
On 2011/07/25 17:37:17, Vitaly Repeshko wrote:
I'd be happy to review this, but
Yes, ObjectDictionary::AddChecked (suggestions for a better name
appreciated)
makes sure each object added to the dictionary has a hash attached to it.
You
are right, lookups of objects not in the table trigger the assertion. I
will fix
that. Thanks!
On 2011/07/25 17:45:17, Vitaly Repeshko
Added the test snippet from Vitaly to cctest. I decided to completely
overload
Dictionary::FindEntry and Dictionary::Add, because the base implementation
should not be called for object dictionaries anyways.
http://codereview.chromium.org/7385006/
--
v8-dev mailing list
v8-dev@googlegroups.com
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.
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
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#newcode11772
src/objects.cc:11772: MaybeObject* maybe_hash = key->GetIdentityHash();
On 2011/07/26 22:30:23, Michael Starzinger wrote:
On
The last set of patches is bigger than expected because of changing the base
class. Also the testcase is more comprehensive. But it should be much
cleaner
now. Sorry for the review-overhead, I am still trying to get to know the
codebase. :)
http://codereview.chromium.org/7385006/diff/7003/src
LGTM
http://codereview.chromium.org/7385006/diff/15001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7385006/diff/15001/src/objects.h#newcode1646
src/objects.h:1646: // Makes sure to properly call BypassGlobalProxy to
be safe for JSGlobalProxy
I don't think the commen
http://codereview.chromium.org/7385006/diff/15001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7385006/diff/15001/src/objects.h#newcode1646
src/objects.h:1646: // Makes sure to properly call BypassGlobalProxy to
be safe for JSGlobalProxy
On 2011/07/28 13:30:07, Vitaly
11 matches
Mail list logo
