https://codereview.chromium.org/220293002/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode15924
src/objects.cc:15924: MaybeObject*
OrderedHashTable<entrysize>::EnsureGrowable() {
On 2014/04/02 13:09:12, rossberg wrote:
On 2014/04/02 12:09:37, Michael Starzinger wrote:
> Is there a particular reason why this implementation is unhandlified
and the
> sub-classes contain handle-wrappers? If not, I would vote for making
this
> implementation be handle-based. IIUC, this would alleviate the need
to even
> declare them explicitly.
>
> Admittedly, doing that straight-forward would make the return type
be
> Handle<OrderedHashTable> instead of the concrete sub-class. But one
idea
> (courtesy of Andreas) would be to add a second template parameter
(in addition
> to "entrysize") to declare to concrete type.
More concretely, something like
template<class Derived, int entrysize> class OrderHashTable {
// ..., e.g.:
Handle<Derived> Rehash(Handle<Derived>);
};
class OrderedHashSet: OrderedHashTable<OrderedHashSet, 1> { ... };
should work (perhaps with some extra tweaks).
Done and done. This code was so-structured because it was based on
ObjectHashTable.
Note that in the current patch Allocate() is also handlified, but this
seems to go against the norms of objects.cc...let me know what's
preferred here.
https://codereview.chromium.org/220293002/diff/1/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/220293002/diff/1/src/objects.h#newcode94
src/objects.h:94: // - MapCache
On 2014/04/02 12:09:37, Michael Starzinger wrote:
nit: Can we add the three new classes into the hierarchy here?
Done.
https://codereview.chromium.org/220293002/diff/1/src/objects.h#newcode4267
src/objects.h:4267: // [3]: "hash table", an array of length
NumberOfBuckets() entry numbers
On 2014/04/02 12:09:37, Michael Starzinger wrote:
nit: Can we somehow visually note that the "hash table" is actually at
[3..n],
because currently it looks like there is an indirection at [3] that
points to
the "hash table".
Explained more. This could probably use some more explanation, though,
I'll see if I can figure a better way to lay it out.
https://codereview.chromium.org/220293002/diff/1/test/cctest/test-ordered-hash-table.cc
File test/cctest/test-ordered-hash-table.cc (right):
https://codereview.chromium.org/220293002/diff/1/test/cctest/test-ordered-hash-table.cc#newcode158
test/cctest/test-ordered-hash-table.cc:158: // BELOW THIS LINE TESTS
WERE CONVERTED FROM test-dictionary.cc
On 2014/04/02 12:09:37, Michael Starzinger wrote:
It would be nice if we could avoid duplicating the tests from
test-dictionary.cc
here. Would it be possible to templatize the tests in
test-dictionary.cc and
instantiate them there for both, ObjectHashTable and OrderedHashMap.
At a first
glance this looks doable for all tests right after the call to
Factory::NewFoo().
Yeah, should be able to take care of this, will look into it next.
https://codereview.chromium.org/220293002/
--
--
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/d/optout.
