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/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/27 15:42:26, Vitaly Repeshko wrote:
On 2011/07/26 22:30:23, Michael Starzinger wrote:
> On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
> > It'd be nice to avoid allocating a hidden properties object in
this case.
> Looks
> > like GetIdentityHash needs create_if_needed too. Since the flag
gets more
> usage
> > our style is to create an enum for it to aid readability.
>
> Fixed (partially, didn't create an enum so far). I suggest turning
it into an
> enum in a separate CL.
Why do you want to add the enum in a separate CL? Seems like a logical
part of
this change to me.
Fixed. Ok. :)
http://codereview.chromium.org/7385006/diff/7003/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2942
src/objects.h:2942: int FindEntry(Isolate* isolate, JSObject* key);
On 2011/07/27 15:42:26, Vitaly Repeshko wrote:
On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
> Doesn't seem to be defined/called. Remove?
Did you miss this one?
Fixed. Yep, missed that one.
http://codereview.chromium.org/7385006/diff/7003/src/objects.h#newcode2946
src/objects.h:2946: MUST_USE_RESULT MaybeObject* Add(JSObject* key,
On 2011/07/27 15:42:26, Vitaly Repeshko wrote:
On 2011/07/26 22:30:23, Michael Starzinger wrote:
> On 2011/07/26 13:53:14, Vitaly Repeshko wrote:
> > Same here.
> >
> > Are PropertyDetails really required? If not, change the entry size
in the
> shape
> > to 2.
>
> In that case the ObjectDictionary should use HashTable instead of
Dictionary
as
> a base class, because those PropertyDetails are the only difference
(besides
> being enumerable) between them. Right?
Yes, Dictionaries are used for storing properties and elements. Change
the base
class. Maybe the name should also be changed to ObjectHashTable.
Fixed. Required a lot of other changes, but it becomes much cleaner now
I think. :)
http://codereview.chromium.org/7385006/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
