Handle optional iterable argument in WeakMap constructor

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: anba, Assigned: arai)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Same issue as in bug 726223, only this time for WeakMaps.

Simple test case:
---
key = {};
wm = new WeakMap([[key, 123]]);
value = wm.get(key);
assertEq(value, 123);
---
(Assignee)

Comment 1

5 years ago
Implemented ES6 23.3.1.1 steps 5-7 and 11-12, except `null` in step 6,
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap-iterable

Also, added almost same testcases as Map-constructor-*.
There are so much differences between them, so I added separetely.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7efe84f31791


For steps 2-4 and 8-9, I leave it to bug 1062075 and bug 804279,
and `null` in step 6 to bug 1092538.
(Both could cause compatibility problem, more likely than this patch)

Then, in step 7a, it gets "set" property of this-value, to call it while iterating,
it means that if we alter `WeakMap.prototype.set` to another function,
WeakMap constructor should call it, right? (what jit-test/tests/collections/WeakMap-constructor-set.js tests)
Similar thing is noted in bug 804279, for generic.
If this is true for non-generic call, other collections's constructor should also be fixed.
If not, i'll remove that part from this patch, and leave it to bug 804279.
Attachment #8516452 - Flags: review?(jorendorff)
(Assignee)

Comment 2

5 years ago
Sorry, I overlooked second example in bug 804279 comment #0.
It's just what I asked about step 7a in comment #1.
I would propose that we should just implement this like all the other Map/Set/WeakSet constructors right now and totally ignore calling any method instead of just doing JS::SetWeakMapEntry. I don't see any fault however with handling null/undefined correctly from the start.
(Assignee)

Comment 4

5 years ago
Okay, thank you for suggestion.
I'll remove step7a things (and any other differences between other containers) for now.
Then try to fix bug 1092538 as a next step.
(Assignee)

Comment 5

5 years ago
Comment on attachment 8516452 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

Clearing r? for now.
I'll post updated patch shortly.
Attachment #8516452 - Flags: review?(jorendorff)
(Assignee)

Comment 6

5 years ago
Removed step7a, will be fixed as bug 804279.
Also, use SetWeakMapEntry instead of SetWeakMapEntryInternal as WeakSet does.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f1fdfb5b74c0
Attachment #8516452 - Attachment is obsolete: true
Attachment #8521385 - Flags: review?(jorendorff)
Comment on attachment 8521385 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

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

Looks really good.

::: js/src/builtin/MapObject.cpp
@@ +1250,5 @@
>              if (done)
>                  break;
>              if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "map");

Map

::: js/src/jsweakmap.cpp
@@ +548,5 @@
> +
> +            // Step 12f.
> +            if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "weakmap");

WeakMap
Attachment #8521385 - Flags: review?(jorendorff) → review?(evilpies)
Attachment #8521385 - Flags: review?(evilpies) → review+
Thanks arai, nice work! I landed this for you, here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae8ccc52200
(Assignee)

Comment 9

5 years ago
Thank you!
https://hg.mozilla.org/mozilla-central/rev/dae8ccc52200
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.