Updated patch set.
http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc File src/bootstrapper.cc (right): http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2180 src/bootstrapper.cc:2180: if (FLAG_harmony_weakmaps) { // -- W e a k M a p On 2011/08/02 12:33:38, rossberg wrote:
Can you perhaps move this into a separate function
InitializeExperimentalGlobal? Done. I suppose you meant InstallExperimentalNatives, right? http://codereview.chromium.org/7529007/diff/4001/src/bootstrapper.cc#newcode2185 src/bootstrapper.cc:2185: //global_context()->set_weakmap_function(*weakmap_fun); On 2011/08/02 12:48:02, danno wrote:
Should this really be commented out? If the code isn't used, it's
better to
remove it.
Done. http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h File src/flag-definitions.h (right): http://codereview.chromium.org/7529007/diff/4001/src/flag-definitions.h#newcode101 src/flag-definitions.h:101: DEFINE_bool(harmony_weakmaps, false, "enable harmony weak-maps") On 2011/08/02 12:33:38, rossberg wrote:
Nit: use space instead of hyphen.
Done. http://codereview.chromium.org/7529007/diff/4001/src/messages.js File src/messages.js (right): http://codereview.chromium.org/7529007/diff/4001/src/messages.js#newcode204 src/messages.js:204: invalid_weakmap_key: ["Invalid value used as weak-map key"], On 2011/08/02 12:33:38, rossberg wrote:
Nit: space instead of hyphen.
Done http://codereview.chromium.org/7529007/diff/4001/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7529007/diff/4001/src/objects.h#newcode6648 src/objects.h:6648: DISALLOW_IMPLICIT_CONSTRUCTORS(JSWeakMap); On 2011/08/02 12:48:02, danno wrote:
I wonder, do you really properly handle in-line properties here? Or do
you
accidentally overwrite them?
Yes, any in-object properties would overwrite the inlined "table" field in this case. I just verified that weakmap objects have zero in-object properties and added the following assertion to the initialization method: ASSERT(weakmap->map()->inobject_properties() == 0); http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/7529007/diff/4001/src/runtime.cc#newcode638 src/runtime.cc:638: RUNTIME_FUNCTION(MaybeObject*, Runtime_WeakMapCreate) { On 2011/08/02 12:33:38, rossberg wrote:
Since unlike the others, this function does not actually create the
object, but
only initializes it, I would prefer calling it WeakMapInitialize or WeakMapConstruct.
Done. http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js File src/weakmap.js (right): http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode43 src/weakmap.js:43: if (!IS_OBJECT(key)) { On 2011/08/02 12:33:38, rossberg wrote:
You probably want IS_SPEC_OBJECT here (and below).
Done. http://codereview.chromium.org/7529007/diff/4001/src/weakmap.js#newcode61 src/weakmap.js:61: //%SetProperty($WeakMap.prototype, "constructor", $WeakMap, DONT_ENUM); On 2011/08/02 12:33:38, rossberg wrote:
Is there a reason that this is commented out?
Done. http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmaps.js File test/mjsunit/harmony/weakmaps.js (right): http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmaps.js#newcode55 test/mjsunit/harmony/weakmaps.js:55: TestMapping(m, new Object, new Object); On 2011/08/02 12:33:38, rossberg wrote:
Does this work with proxies as keys?
No, actually it doesn't. Unfortunately the whole concept of identity hash codes is implemented on JSObject, but a JSProxy cannot be cast to a JSObject. So it will require quite some effort to make it work. Does the Harmony proposal for proxies say anything about identity hash codes for proxies? http://codereview.chromium.org/7529007/diff/4001/test/mjsunit/harmony/weakmaps.js#newcode96 test/mjsunit/harmony/weakmaps.js:96: assertTrue(WeakMap.prototype.get instanceof Function) On 2011/08/02 12:48:02, danno wrote:
add test cases that set and get arbitrary properties and elements on a
weak map,
especially ones designed to trigger fast in-object properties.
Done. http://codereview.chromium.org/7529007/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
