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; Not needed? 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, I think you can remove the separate hormony_weakmaps flag and subsume it under harmony_collections. 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; Test the common case first. Also, you can condense these two lines to return this == that || (isnan(this) && isnan(that)); http://codereview.chromium.org/8372027/diff/1/src/objects.cc#newcode12229 src/objects.cc:12229: if (key->IsJSReceiver()) { For example, this conditional is redundant if GetHash created a hash. (Same in Put.) 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(); 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. 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 { 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. http://codereview.chromium.org/8372027/diff/1/src/runtime.cc#newcode776 src/runtime.cc:776: CONVERT_ARG_CHECKED(JSReceiver, key, 1); Why is the key still a JSReceiver here (and below)? 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) I don't see any proper test cases for maps (especially, I think you will still fail on non-object keys, see runtime.cc). 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)); 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.) http://codereview.chromium.org/8372027/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
