ES6: Adding support for size to Set and Map (issue 11360089)

[a...@chromium.org]

Reviewers: Sven Panne, rossberg,

Description:
Adding support for size to Set and Map

Section 15.14.5.10 and 15.16.5.7 in the October 26, 2012 ES6 draft,
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts

This adds a getter for "size" to Set.prototype and Map.prototype which
reflects
the number of elements in the Set and Map respectively.

BUG=2395


Please review this at https://codereview.chromium.org/11360089/

SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/collection.js
M src/runtime.h
M src/runtime.cc
M test/mjsunit/harmony/collections.js


Index: src/collection.js
diff --git a/src/collection.js b/src/collection.js
index
d36fe18fa00dcd5fdae1210e5f9470c53cbc41f3..f8e3594bb67a8623d8ce63a97f04412710f43a8d
100644
--- a/src/collection.js
+++ b/src/collection.js
@@ -88,6 +88,15 @@ function SetDelete(key) {
}


+function SetSize() {
+ if (!IS_SET(this)) {
+ throw MakeTypeError('incompatible_method_receiver',
+ ['Set.prototype.size', this]);
+ }
+ return %SetSize(this);
+}
+
+
function MapConstructor() {
if (%_IsConstructCall()) {
%MapInitialize(this);
@@ -145,6 +154,15 @@ function MapDelete(key) {
}


+function MapSize() {
+ if (!IS_MAP(this)) {
+ throw MakeTypeError('incompatible_method_receiver',
+ ['Map.prototype.size', this]);
+ }
+ return %MapSize(this);
+}
+
+
function WeakMapConstructor() {
if (%_IsConstructCall()) {
%WeakMapInitialize(this);
@@ -214,7 +232,16 @@ function WeakMapDelete(key) {
%SetProperty($Set.prototype, "constructor", $Set, DONT_ENUM);
%SetProperty($Map.prototype, "constructor", $Map, DONT_ENUM);

+ function DefineGetter(object, name, fun) {
+ %FunctionSetName(fun, name);
+ var desc = new PropertyDescriptor();
+ desc.setGet(fun);
+ desc.setConfigurable(true);
+ DefineObjectProperty(object, name, desc, false);
+ }
+
// Set up the non-enumerable functions on the Set prototype object.
+ DefineGetter($Set.prototype, "size", SetSize);
InstallFunctions($Set.prototype, DONT_ENUM, $Array(
"add", SetAdd,
"has", SetHas,
@@ -222,6 +249,7 @@ function WeakMapDelete(key) {
));

// Set up the non-enumerable functions on the Map prototype object.
+ DefineGetter($Map.prototype, "size", MapSize);
InstallFunctions($Map.prototype, DONT_ENUM, $Array(
"get", MapGet,
"set", MapSet,
Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index
b0a6b5e81427e1588a89f1c269a2b1e90c78f98b..861d79f630be5b43f5f122396b2afb0250c2bc1a
100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -783,6 +783,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_SetDelete) {
}


+RUNTIME_FUNCTION(MaybeObject*, Runtime_SetSize) {
+ HandleScope scope(isolate);
+ ASSERT(args.length() == 1);
+ CONVERT_ARG_HANDLE_CHECKED(JSSet, holder, 0);
+ Handle<ObjectHashSet> table(ObjectHashSet::cast(holder->table()));
+ return Smi::FromInt(table->NumberOfElements());
+}
+
+
RUNTIME_FUNCTION(MaybeObject*, Runtime_MapInitialize) {
HandleScope scope(isolate);
ASSERT(args.length() == 1);
@@ -842,6 +851,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_MapSet) {
}


+RUNTIME_FUNCTION(MaybeObject*, Runtime_MapSize) {
+ HandleScope scope(isolate);
+ ASSERT(args.length() == 1);
+ CONVERT_ARG_HANDLE_CHECKED(JSMap, holder, 0);
+ Handle<ObjectHashTable> table(ObjectHashTable::cast(holder->table()));
+ return Smi::FromInt(table->NumberOfElements());
+}
+
+
RUNTIME_FUNCTION(MaybeObject*, Runtime_WeakMapInitialize) {
HandleScope scope(isolate);
ASSERT(args.length() == 1);
Index: src/runtime.h
diff --git a/src/runtime.h b/src/runtime.h
index
c8029065f6908ce9f5d04c695dea00ffb098d687..d2126c108a7b2884ad5fd106525b424d779d2196
100644
--- a/src/runtime.h
+++ b/src/runtime.h
@@ -302,6 +302,7 @@ namespace internal {
F(SetAdd, 2, 1) \
F(SetHas, 2, 1) \
F(SetDelete, 2, 1) \
+ F(SetSize, 1, 1) \
\
/* Harmony maps */ \
F(MapInitialize, 1, 1) \
@@ -309,6 +310,7 @@ namespace internal {
F(MapHas, 2, 1) \
F(MapDelete, 2, 1) \
F(MapSet, 3, 1) \
+ F(MapSize, 1, 1) \
\
/* Harmony weakmaps */ \
F(WeakMapInitialize, 1, 1) \
Index: test/mjsunit/harmony/collections.js
diff --git a/test/mjsunit/harmony/collections.js
b/test/mjsunit/harmony/collections.js
index
f3db7ea2b70a9670fa7e63082b2022bbd57f2576..2b9fe3e6f094e088d412b9b93603c5987f52e717
100644
--- a/test/mjsunit/harmony/collections.js
+++ b/test/mjsunit/harmony/collections.js
@@ -313,4 +313,43 @@ TestBogusReceivers(bogusReceiversTestSet);
// Stress Test
// There is a proposed stress-test available at the es-discuss mailing list
// which cannot be reasonably automated. Check it out by hand if you like:
-// https://mail.mozilla.org/pipermail/es-discuss/2011-May/014096.html
\ No newline at end of file
+// https://mail.mozilla.org/pipermail/es-discuss/2011-May/014096.html
+
+
+// Set and Map Set size
+var setSizeDescriptor =
Object.getOwnPropertyDescriptor(Set.prototype, 'size');
+assertEquals(undefined, setSizeDescriptor.value);
+assertEquals(undefined, setSizeDescriptor.set);
+assertTrue(setSizeDescriptor.get instanceof Function);
+assertFalse(setSizeDescriptor.enumerable);
+assertTrue(setSizeDescriptor.configurable);
+
+var s = new Set();
+assertFalse(s.hasOwnProperty('size'));
+for (var i = 0; i < 10; i++) {
+ assertEquals(i, s.size);
+ s.add(i);
+}
+for (var i = 9; i >= 0; i--) {
+ s.delete(i);
+ assertEquals(i, s.size);
+}
+
+
+var mapSizeDescriptor =
Object.getOwnPropertyDescriptor(Map.prototype, 'size');
+assertEquals(undefined, mapSizeDescriptor.value);
+assertEquals(undefined, mapSizeDescriptor.set);
+assertTrue(mapSizeDescriptor.get instanceof Function);
+assertFalse(mapSizeDescriptor.enumerable);
+assertTrue(mapSizeDescriptor.configurable);
+
+var m = new Map();
+assertFalse(m.hasOwnProperty('size'));
+for (var i = 0; i < 10; i++) {
+ assertEquals(i, m.size);
+ m.set(i, i);
+}
+for (var i = 9; i >= 0; i--) {
+ m.delete(i);
+ assertEquals(i, m.size);
+}

[a...@chromium.org]

-Sven, +Michael

I intended mstarzinger to review this since he implemented Set and Map
initially.

https://codereview.chromium.org/11360089/

[mstar...@chromium.org]

This is already looking good. Just a few comments.


https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js
File src/collection.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js#newcode91
src/collection.js:91: function SetSize() {
I think "SetGetSize" or "SetSizeGetter" would be a better naming
convention for getters.

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js#newcode157
src/collection.js:157: function MapSize() {
Likewise for "MapGetSize" or "MapSizeGetter".

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js#newcode235
src/collection.js:235: function DefineGetter(object, name, fun) {
Can we move this into v8natives.js near the implementation of
InstallFunctions, so that it can become the bottle-neck for all builtin
getters that we install?

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js
File test/mjsunit/harmony/collections.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode319
test/mjsunit/harmony/collections.js:319: // Set and Map Set size
Comment seems to contain a typo.

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode324
test/mjsunit/harmony/collections.js:324:
assertFalse(setSizeDescriptor.enumerable);
I couldn't find the expected value of the attributes (i.e. enumerable
and configurable) in the draft spec. Is that still unspeced?

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode343
test/mjsunit/harmony/collections.js:343:
assertFalse(mapSizeDescriptor.enumerable);
Likewise.

https://chromiumcodereview.appspot.com/11360089/

[a...@chromium.org]

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js
File src/collection.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js#newcode91
src/collection.js:91: function SetSize() {

On 2012/11/06 09:32:40, Michael Starzinger wrote:
> I think "SetGetSize" or "SetSizeGetter" would be a better naming
convention for
> getters.

Done.

https://chromiumcodereview.appspot.com/11360089/diff/2001/src/collection.js#newcode235
src/collection.js:235: function DefineGetter(object, name, fun) {

On 2012/11/06 09:32:40, Michael Starzinger wrote:
> Can we move this into v8natives.js near the implementation of
InstallFunctions,
> so that it can become the bottle-neck for all builtin getters that we
install?

Done.

I also used a lower level abstraction.

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js
File test/mjsunit/harmony/collections.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode319
test/mjsunit/harmony/collections.js:319: // Set and Map Set size

On 2012/11/06 09:32:40, Michael Starzinger wrote:
> Comment seems to contain a typo.

Done.

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode324
test/mjsunit/harmony/collections.js:324:
assertFalse(setSizeDescriptor.enumerable);

On 2012/11/06 09:32:40, Michael Starzinger wrote:
> I couldn't find the expected value of the attributes (i.e. enumerable
and
> configurable) in the draft spec. Is that still unspeced?

Last sentence of last paragraph in the introduction to clause 15:

Every other property described in this clause has the attributes {
[[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true }
unless otherwise specified.

https://chromiumcodereview.appspot.com/11360089/diff/6/src/v8natives.js
File src/v8natives.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/6/src/v8natives.js#newcode67
src/v8natives.js:67: %FunctionRemovePrototype(getter);
I missed this one last time. The spec says that all spec functions
unless, otherwise stated, should NOT have a prototype property.

https://chromiumcodereview.appspot.com/11360089/

[mstar...@chromium.org]

LGTM. I'll land this for you.

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js
File test/mjsunit/harmony/collections.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode324
test/mjsunit/harmony/collections.js:324:
assertFalse(setSizeDescriptor.enumerable);

On 2012/11/06 16:02:33, arv wrote:
> On 2012/11/06 09:32:40, Michael Starzinger wrote:
> > I couldn't find the expected value of the attributes (i.e.
enumerable and
> > configurable) in the draft spec. Is that still unspeced?

> Last sentence of last paragraph in the introduction to clause 15:

> Every other property described in this clause has the attributes {
[[Writable]]:
> true, [[Enumerable]]: false, [[Configurable]]: true } unless otherwise
> specified.

Ah, yes, I didn't see this sentence. To be pedantic (as Andreas pointed
out), this sentence is talking about data properties (as it contains the
writable attribute) and it's unclear whether is also applies to accessor
properties. But I am fine to land this CL as it is even without a
clarification on this part.

https://chromiumcodereview.appspot.com/11360089/

[a...@chromium.org]

Thanks.

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js
File test/mjsunit/harmony/collections.js (right):

https://chromiumcodereview.appspot.com/11360089/diff/2001/test/mjsunit/harmony/collections.js#newcode324
test/mjsunit/harmony/collections.js:324:
assertFalse(setSizeDescriptor.enumerable);

On 2012/11/06 17:47:55, Michael Starzinger wrote:
> On 2012/11/06 16:02:33, arv wrote:
> > On 2012/11/06 09:32:40, Michael Starzinger wrote:
> > > I couldn't find the expected value of the attributes (i.e.
enumerable and
> > > configurable) in the draft spec. Is that still unspeced?
> >
> > Last sentence of last paragraph in the introduction to clause 15:
> >
> > Every other property described in this clause has the attributes {
> [[Writable]]:
> > true, [[Enumerable]]: false, [[Configurable]]: true } unless
otherwise
> > specified.

> Ah, yes, I didn't see this sentence. To be pedantic (as Andreas
pointed out),
> this sentence is talking about data properties (as it contains the
writable
> attribute) and it's unclear whether is also applies to accessor
properties. But
> I am fine to land this CL as it is even without a clarification on
this part.

I had a hard time finding this too. I even filed an ES6 bug before Allen
pointed out where to find this.

https://bugs.ecmascript.org/show_bug.cgi?id=931

https://chromiumcodereview.appspot.com/11360089/