Added new patch set. PTAL.
http://codereview.chromium.org/8372027/diff/1/src/collection.js File src/collection.js (right): http://codereview.chromium.org/8372027/diff/1/src/collection.js#newcode31 src/collection.js:31: // const $Object = global.Object; On 2011/10/24 15:44:11, rossberg wrote:
Not needed?
Done. http://codereview.chromium.org/8372027/diff/1/src/flag-definitions.h File src/flag-definitions.h (right): http://codereview.chromium.org/8372027/diff/1/src/flag-definitions.h#newcode103 src/flag-definitions.h:103: DEFINE_bool(harmony_collections, false, On 2011/10/24 15:44:11, rossberg wrote:
I think you can remove the separate hormony_weakmaps flag and subsume
it under
harmony_collections.
Done. In this case it might be useful to move the WeakMap implementation from the weakmap.js file into the collection.js file for consistency reasons. http://codereview.chromium.org/8372027/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8372027/diff/1/src/objects.cc#newcode754 src/objects.cc:754: return this_value == other_value; On 2011/10/24 15:44:11, rossberg wrote:
Test the common case first. Also, you can condense these two lines to
return this == that || (isnan(this) && isnan(that));
Done. http://codereview.chromium.org/8372027/diff/1/src/objects.cc#newcode12229 src/objects.cc:12229: if (key->IsJSReceiver()) { On 2011/10/24 15:44:11, rossberg wrote:
For example, this conditional is redundant if GetHash created a hash.
(Same in
Put.)
Done (see comment in objects.h). http://codereview.chromium.org/8372027/diff/1/src/objects.h File src/objects.h (right): http://codereview.chromium.org/8372027/diff/1/src/objects.h#newcode947 src/objects.h:947: uint32_t GetHash(); On 2011/10/24 15:44:11, rossberg wrote:
It is not very intuitive that this function fails on a JSReceiver that
does not
have a hash yet. Is this ever useful behaviour? At the very least, you
should
document it.
But I think it's preferable to always create it - and you could avoid
a couple
of extra cases and repeated hash lookups in your code.
Done. As discussed extensively offline, the only feasible solution to prevent special handling of JSReceiver at the caller site it to return MaybeObject. This limits the space of available hash codes to 31 bit. Always creating it is not an option because a GC might be caused when storing the hash as a hidden property. http://codereview.chromium.org/8372027/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/8372027/diff/1/src/runtime.cc#newcode731 src/runtime.cc:731: enum SetAccessOperation { On 2011/10/24 15:44:11, rossberg wrote:
I'm not sure I like this. The amount of code it safes is far too tiny
to warrant
introducing a special construction like this.
Done. http://codereview.chromium.org/8372027/diff/1/src/runtime.cc#newcode776 src/runtime.cc:776: CONVERT_ARG_CHECKED(JSReceiver, key, 1); On 2011/10/24 15:44:11, rossberg wrote:
Why is the key still a JSReceiver here (and below)?
Done (fixed by second patch set). http://codereview.chromium.org/8372027/diff/1/test/mjsunit/harmony/collections.js File test/mjsunit/harmony/collections.js (right): http://codereview.chromium.org/8372027/diff/1/test/mjsunit/harmony/collections.js#newcode63 test/mjsunit/harmony/collections.js:63: assertTrue(Map.prototype.delete instanceof Function) On 2011/10/24 15:44:11, rossberg wrote:
I don't see any proper test cases for maps (especially, I think you
will still
fail on non-object keys, see runtime.cc).
Done. http://codereview.chromium.org/8372027/diff/1/test/mjsunit/harmony/proxies-hash.js File test/mjsunit/harmony/proxies-hash.js (right): http://codereview.chromium.org/8372027/diff/1/test/mjsunit/harmony/proxies-hash.js#newcode90 test/mjsunit/harmony/proxies-hash.js:90: assertSame(321, m.get(p2)); On 2011/10/24 15:44:11, rossberg wrote:
Now that you have the special code paths for non-existing hashs, can
you add
tests for .has and .delete? (The latter also for sets.)
Done (I suppose you mean non-existing keys). http://codereview.chromium.org/8372027/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
