I agree with Andreas' comment, and also have some of my own. We probably
should
add a comment to the definition of MapMirror.entries stating that it will
keep
keys alive for WeakMaps.
https://codereview.chromium.org/398513005/diff/1/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/398513005/diff/1/src/runtime.cc#newcode1742
src/runtime.cc:1742: Handle<Object> key(table->KeyAt(i), isolate);
Indentation size is weird. We use 4 spaces for line break and 2 spaces
for scopes. You could also use "git cl format" to have it formatted for
you.
https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-collections.js
File test/mjsunit/harmony/mirror-collections.js (right):
https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-collections.js#newcode9
test/mjsunit/harmony/mirror-collections.js:9: var serializer =
debug.MakeMirrorSerializer();
Same here. 2 space indentation please.
https://codereview.chromium.org/398513005/diff/1/test/mjsunit/harmony/mirror-collections.js#newcode51
test/mjsunit/harmony/mirror-collections.js:51: assertEquals(44,
entries[5]);
Having MapMirror.entries() contain both keys and value is somewhat
confusing. It would be nicer to have an array of key/value pairs. That
way, entries.length will also not be twice the actual number of added
mappings.
On the other hand that would add some more overhead when preparing that
array.
We could have methods on MapMirror that would abstract away from direct
access to entries, but then it becomes unclear when to refresh the
entries array.
:/
https://codereview.chromium.org/398513005/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
