Looking good, I like this. The bulk of the comments is about handles vs. raw
pointers.
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() {
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.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode15942
src/objects.cc:15942: MaybeObject* OrderedHashTable<entrysize>::Shrink()
{
Likewise.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode15956
src/objects.cc:15956: MaybeObject*
OrderedHashTable<entrysize>::Rehash(OrderedHashTable* new_table) {
AFAICT, addressing the above two comments would also allow us to
implement this one handle-based.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode16031
src/objects.cc:16031: Handle<OrderedHashSet>
OrderedHashSet::EnsureGrowable(
Obsolete if above comments are addressed, I think.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode16040
src/objects.cc:16040: Handle<OrderedHashSet>
OrderedHashSet::Shrink(Handle<OrderedHashSet> table) {
Likewise.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode16078
src/objects.cc:16078: Handle<OrderedHashMap>
OrderedHashMap::EnsureGrowable(
Likewise.
https://codereview.chromium.org/220293002/diff/1/src/objects.cc#newcode16087
src/objects.cc:16087: Handle<OrderedHashMap>
OrderedHashMap::Shrink(Handle<OrderedHashMap> table) {
Likewise.
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
nit: Can we add the three new classes into the hierarchy here?
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
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".
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
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().
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.
