Last Comment Bug 726223 - Map and Set constructors should take a single optional iterable argument
: Map and Set constructors should take a single optional iterable argument
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-10 16:24 PST by Jason Orendorff [:jorendorff]
Modified: 2012-06-17 15:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (10.17 KB, patch)
2012-02-10 16:25 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (17.21 KB, patch)
2012-02-11 07:50 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v3 (17.44 KB, patch)
2012-02-12 06:27 PST, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-02-10 16:24:24 PST
Set(iterable) should effectively do this:

    for (let v of iterable)
        the_new_set.add(v);

Likewise, Map(iterable) should do this:

    for (let [k, v] of iterable)
        the_new_map.set(k, v);

A nice thing about this is that if we also make Map iteration yield pairs, then it provides a fairly nice way to make a copy of a Map or set:

    new Map(oldmap)
    new Set(oldset)

None of this has been mentioned in TC39 or even on es-discuss.
Comment 1 Jason Orendorff [:jorendorff] 2012-02-10 16:25:29 PST
Created attachment 596218 [details] [diff] [review]
WIP 1

needs more tests
Comment 2 Jason Orendorff [:jorendorff] 2012-02-11 07:50:57 PST
Created attachment 596329 [details] [diff] [review]
v2
Comment 3 Jason Orendorff [:jorendorff] 2012-02-11 07:52:03 PST
For dherman:

From an API design perspective, there are two things I'm mildly uncertain about:

1. Another reasonable thing folks might want is for
    Map({x: 1})
to produce a Map with .get("x") === 1. Currently it just throws.

We can maybe satisfy that need with a helper:
    Map.fromProperties({x: 1})
or perhaps:
    import * from "@inspect";
    Map(properties({x: 1}))

Or else Allen's object reform ideas which I still haven't looked at.

2. This is not very strict about the input. Map(DATA) behaves exactly like:
    m = Map();
    for (let [k, v] of DATA)
        m.set(k, v);
and destructuring is quite lenient. So all these errors pass silently:
    // A string can be a pair
    Map(["hello"])              // ===> #{"h" => "e"}#

    // An object that clearly isn't a pair can be a pair
    Map([{}])                   // ===> #{undefined => undefined}#

    // Simpsons nuclear fish case: a triple can be a pair
    Map([[key, value, blah]])   // ===> #{key => value}#

But if this is a flaw, it is a flaw in destructuring, not this patch per se.
Comment 4 Jason Orendorff [:jorendorff] 2012-02-12 06:27:54 PST
Created attachment 596472 [details] [diff] [review]
v3

Fix OOM reporting in AddToMap and AddToSet.
Comment 5 Luke Wagner [:luke] 2012-02-20 12:41:11 PST
Comment on attachment 596472 [details] [diff] [review]
v3

Review of attachment 596472 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the JS implementation side assuming you flag brendan/dherman/allen for an API review.  I dare say my JS cultural knowledge is weak.  (On the subject, though: I naively expected Map(obj) to map obj's properties to values.  On further reflection, though, I see why arrays are preferable so I like the Map.fromProperties idea.)

Beautiful patch; one request:

::: js/src/builtin/MapObject.cpp
@@ +250,5 @@
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Value arg = (argc > 0 ? args[0] : UndefinedValue());
> +    if (!arg.isUndefined()) {
> +        if (!ForOf(cx, arg, AddToMap(map)))

This "if we have an arg and it isn't undefined" pattern seems to come up a lot and always leads to predicates like this.  It seems like we could add a 'hasDefined(n)' that lets us write:

  if (args.hasDefined(0) && !ForOf(cx, args[0], AddToMap(map))
      return false;

A quick grep for /arg.*isUndefined/ shows 20 or places that could use this!
Comment 6 Jason Orendorff [:jorendorff] 2012-02-25 13:50:59 PST
Dave Herman and Mark Miller are OK with this API, at least provisionally. (Mark is the design lead for this feature in TC39.) Of course we'll implement whatever TC39 settles on in the end.

Follow-up bug for hasDefined is coming up.
Comment 7 Jason Orendorff [:jorendorff] 2012-02-26 07:48:36 PST
Filed bug 730676 with patch.
Comment 8 Jason Orendorff [:jorendorff] 2012-03-01 07:13:42 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b944a4b6230

Bug 730676 will land shortly; apparently one of these patches I'm trying to land has a small Linux-only, valgrind-only bug that I'm still trying to track down.
Comment 9 Marco Bonardo [::mak] 2012-03-02 06:20:48 PST
https://hg.mozilla.org/mozilla-central/rev/9b944a4b6230

Note You need to log in before you can comment on or make changes to this bug.