Map/Set: Implement constructor parameter handling

Map/Set: Implement constructor parameter handling

When an iterable object is passed in as the argument to the Map and Set
constructor the elements of the iterable object are used to populate the
Map and Set.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-map-iterable
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-set-iterable

BUG=v8:3398
LOG=Y
R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21950

[adamk, 2014-06-18 20:43:27]

Code looks fine (especially so since it closely mirrors the spec), one test
suggestion.

https://codereview.chromium.org/345613003/diff/20001/test/mjsunit/harmony/col...
File test/mjsunit/harmony/collections.js (right):

https://codereview.chromium.org/345613003/diff/20001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:1036: (function
TestSetConstructorAddNotCallable() {
Another more complicated version of this test would be nice to have, which
demonstrates that you cache the adder method at the start. E.g., you could wrap
add in another method that replaces itself with 42 on Set.prototype the first
time it's called. And a similar test for Map.

[arv (Not doing code reviews), 2014-06-18 20:51:19]

On 2014/06/18 20:43:27, adamk wrote:
> Code looks fine (especially so since it closely mirrors the spec), one test
> suggestion.
> 
>
https://codereview.chromium.org/345613003/diff/20001/test/mjsunit/harmony/col...
> File test/mjsunit/harmony/collections.js (right):
> 
>
https://codereview.chromium.org/345613003/diff/20001/test/mjsunit/harmony/col...
> test/mjsunit/harmony/collections.js:1036: (function
> TestSetConstructorAddNotCallable() {
> Another more complicated version of this test would be nice to have, which
> demonstrates that you cache the adder method at the start. E.g., you could
wrap
> add in another method that replaces itself with 42 on Set.prototype the first
> time it's called. And a similar test for Map.

Good idea. I will do that.

[arv (Not doing code reviews), 2014-06-18 21:01:16]

Added test where set/add is replaced during set/add

[arv (Not doing code reviews), 2014-06-18 21:02:06]

https://codereview.chromium.org/345613003/diff/50001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/345613003/diff/50001/src/collection.js#newcode15
src/collection.js:15: // 7.4.1 CheckIterable ( obj )
This should probably go in v8natives.js once this file is no longer
experimental.

[arv (Not doing code reviews), 2014-06-18 21:03:04]

Added one more test as suggested.

[rossberg, 2014-06-23 14:16:00]

https://codereview.chromium.org/345613003/diff/50001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/345613003/diff/50001/src/collection.js#newcode15
src/collection.js:15: // 7.4.1 CheckIterable ( obj )
On 2014/06/18 21:02:05, arv wrote:
> This should probably go in v8natives.js once this file is no longer
> experimental.

Agreed. Maybe add a TODO to that end?

We could also consider calling it ToIterable or something -- the spec name is
really poorly chosen, and inconsistent with all other conversions. (I already
filed that with one of my chapter bugs.)

https://codereview.chromium.org/345613003/diff/50001/src/collection.js#newcod...
src/collection.js:187: throw MakeTypeError('not_an_object', [next]);
We typically give more specific error messages in cases like these, where the
type error is internal to some library function. Perhaps e.g. 'iterator result
... is not an object', or (below) 'iterator value ... is not an entry'.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
File test/mjsunit/harmony/collections.js (right):

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:32: var originalSetPrototypeAdd =
Set.prototype.add;
Nit: I'd prefer to localise these declarations into the respective tests, so
that they are self-contained.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:1037: var twoSet = new Set();
Nit: can't you simply use the array [1, 2] here? (Similarly, below.)

In particular, this would also test that the constructors are properly generic.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:1127: });
Assert TypeError, specifically.

[arv (Not doing code reviews), 2014-06-23 15:36:36]

Code review updates

[arv (Not doing code reviews), 2014-06-23 15:40:35]

fix typo

[arv (Not doing code reviews), 2014-06-23 15:42:08]

ascii art

[arv (Not doing code reviews), 2014-06-23 15:43:57]

PTAL

https://codereview.chromium.org/345613003/diff/50001/src/collection.js
File src/collection.js (right):

https://codereview.chromium.org/345613003/diff/50001/src/collection.js#newcode15
src/collection.js:15: // 7.4.1 CheckIterable ( obj )
On 2014/06/23 14:16:00, rossberg wrote:
> On 2014/06/18 21:02:05, arv wrote:
> > This should probably go in v8natives.js once this file is no longer
> > experimental.
> 
> Agreed. Maybe add a TODO to that end?
> 
> We could also consider calling it ToIterable or something -- the spec name is
> really poorly chosen, and inconsistent with all other conversions. (I already
> filed that with one of my chapter bugs.)

Done.

https://codereview.chromium.org/345613003/diff/50001/src/collection.js#newcod...
src/collection.js:187: throw MakeTypeError('not_an_object', [next]);
On 2014/06/23 14:15:59, rossberg wrote:
> We typically give more specific error messages in cases like these, where the
> type error is internal to some library function. Perhaps e.g. 'iterator result
> ... is not an object', or (below) 'iterator value ... is not an entry'.

Great idea.

Done.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
File test/mjsunit/harmony/collections.js (right):

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:32: var originalSetPrototypeAdd =
Set.prototype.add;
On 2014/06/23 14:16:00, rossberg wrote:
> Nit: I'd prefer to localise these declarations into the respective tests, so
> that they are self-contained.

Done.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:1037: var twoSet = new Set();
On 2014/06/23 14:16:00, rossberg wrote:
> Nit: can't you simply use the array [1, 2] here? (Similarly, below.)
> 
> In particular, this would also test that the constructors are properly
generic.

OK. I didn't do that because I didn't want a dependency on --harmony-iteration
but I changed it based on your request.

Note that we have not yet landed Array.prototype[Symbol.iterator] so these will
have to use .values() for now.

https://codereview.chromium.org/345613003/diff/50001/test/mjsunit/harmony/col...
test/mjsunit/harmony/collections.js:1127: });
On 2014/06/23 14:16:00, rossberg wrote:
> Assert TypeError, specifically.

Done.

[rossberg, 2014-06-23 16:12:37]

lgtm

[arv (Not doing code reviews), 2014-06-23 16:14:57]

Would you mind landing this when you have time?

Thanks.

[adamk, 2014-06-23 18:06:03]

Committed patchset #6 manually as r21950 (presubmit successful).