Closed Bug 1092537 Opened 9 years ago Closed 9 years ago

Handle optional iterable argument in WeakMap constructor


(Core :: JavaScript: Standard Library, defect)

Not set





(Reporter: anba, Assigned: arai)




(1 file, 1 obsolete file)

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);
Implemented ES6 steps 5-7 and 11-12, except `null` in step 6,

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

Green on try run:

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)
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.
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.
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)
Removed step7a, will be fixed as bug 804279.
Also, use SetWeakMapEntry instead of SetWeakMapEntryInternal as WeakSet does.

Green on try run:
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");


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

Attachment #8521385 - Flags: review?(jorendorff) → review?(evilpies)
Attachment #8521385 - Flags: review?(evilpies) → review+
Thanks arai, nice work! I landed this for you, here:
Thank you!
Assignee: nobody → arai_a
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.