LGTM, just two nits left.
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 22:01:10, adamk wrote:
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.
I definitely prefer handlified as you did in your recent patch set.
https://codereview.chromium.org/220293002/diff/80001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/220293002/diff/80001/src/objects.cc#newcode15900
src/objects.cc:15900: // capacity must be a factor of two
nit: Comment says "factor", code says "power". Also please capitalize
and punctuate comment.
https://codereview.chromium.org/220293002/diff/80001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/220293002/diff/80001/src/objects.h#newcode4294
src/objects.h:4294: static OrderedHashTable* cast(Object* obj) {
nit: Since this can be considered an abstract class and the two concrete
sub-classes both have their own casting method, should we even have this
casting method? If it is actually used/needed anywhere I am fine with it
of course.
https://codereview.chromium.org/220293002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
