Looking good already. Just a couple of minor comments.
https://codereview.chromium.org/48913008/diff/100001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode1515
src/objects.h:1515: static Handle<Object> GetHash(Handle<Object> object,
Isolate* isolate,
nit: The current implementation as a wrapper requires the isolate
parameter. Once the implementation is fully handlified the isolate
parameter is no longer needed. Let's add a TODO to keep that in mind.
https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4066
src/objects.h:4066: static bool Contains(Handle<ObjectHashSet> table,
Handle<Object> key);
The ObjectHashSet::Contains method should never cause an allocation, it
should be possible to leave this one un-handlified.
https://codereview.chromium.org/48913008/diff/100001/src/objects.h#newcode4068
src/objects.h:4068: static Handle<ObjectHashSet> EnsureSetCapacity(
On 2013/10/29 19:14:51, rafaelw wrote:
Note that I've named this "EnsureSetCapacity" because I can't figure
out how to
get the C++ compiler to stop complaining because in the
CALL_HEAP_FUNCTION
implementation, it seems to be trying to invoke the static method
(forgetting
about the inherted templatized method from HashTable.
I'm hoping you know what the solution to this is. Same problem below
with
ShrinkSet.
One way to address that I can think of would be to add an explicit
up-cast into the implementation of ObjectHashSet::EnsureSetCapacity like
this ...
Handle<HashTable<ObjectHashTableShape<1>, Object*> > table_base = table;
Unfortunately this will lead to access violations because
HashTable::EnsureSetCapacity is protected. But I think we can bake
ObjectHashSet a friend of HashTable to resolve this.
https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-dictionary.cc
File test/cctest/test-dictionary.cc (right):
https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-dictionary.cc#newcode178
test/cctest/test-dictionary.cc:178: int gc_count =
isolate->heap()->gc_count();
On 2013/10/29 19:14:51, rafaelw wrote:
I just took a guess at how to address this test.
Yes, I think using the GC-count makes sense.
https://codereview.chromium.org/48913008/diff/100001/test/cctest/test-dictionary.cc#newcode182
test/cctest/test-dictionary.cc:182: // Calling Remove() should not cause
GC ever.
On 2013/10/29 19:14:51, rafaelw wrote:
This comment doesn't seem true. I believe it's always true for this
test, but
Remove() can clearly allocate more memory via Shrink.
You are right, the comment only holds for a HashSet of this particular
size. Maybe we should adapt the comment accordingly.
https://codereview.chromium.org/48913008/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
