Closed Bug 814562 Opened 7 years ago Closed 7 years ago

Implement clear() on WeakMaps

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: sawrubh)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

Unless there's a reason we don't have that?
I think we just don't have it because it is a fairly recent addition. Should be very easy for anybody who understand spec-ese.
Blocks: es6
Keywords: dev-doc-needed
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: general → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #685623 - Flags: feedback?(jwalden+bmo)
Comment on attachment 685623 [details] [diff] [review]
Patch v1

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

Looks pretty good to me.  This needs a few tests before it can land, tho -- make sure clearing an already-empty WeakMap works, make sure clearing one with a live key works, make sure clearing one with a key that's turned to garbage works, and make sure clearing one with a key that's turned to garbage *and* we've done a GC works.

jorendorff might have more ideas for tests here, too.  Plus, since I'm not super-familiar with the WeakMap code, I'm slightly hesitant to take final responsibility for an actual review here -- ask him for that, please.  :-)

::: js/src/jsweakmap.cpp
@@ +153,5 @@
> +    Rooted<JSObject*> thisObj(cx, &args.thisv().toObject());
> +    if (ObjectValueMap *map = GetObjectMap(thisObj)) {
> +        js_delete(map);
> +        thisObj->setPrivate(NULL);
> +        return true;

Erm, I don't think you need to return at all here, just fall through -- indeed it'd be wrong to return true here, because then you won't be setting the right return value.

@@ +365,5 @@
>  };
>  
>  static JSFunctionSpec weak_map_methods[] = {
>      JS_FN("has",    WeakMap_has, 1, 0),
> +    JS_FN("clear",    WeakMap_clear, 0, 0),

Misaligned.
Attachment #685623 - Flags: feedback?(jwalden+bmo) → feedback+
Attached patch Patch v2Splinter Review
Attachment #685623 - Attachment is obsolete: true
Attachment #687906 - Flags: review?(jorendorff)
Attached patch Test v1Splinter Review
Attachment #687908 - Flags: review?(jorendorff)
Comment on attachment 687906 [details] [diff] [review]
Patch v2

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

Check in code and tests in a single changeset, please.

::: js/src/jsweakmap.cpp
@@ +177,5 @@
> +    JS_ASSERT(IsWeakMap(args.thisv()));
> +
> +    if (ObjectValueMap *map = GetObjectMap(&args.thisv().toObject())) {
> +        map->clear();
> +    }

Please leave a comment explaining why we can't just js_delete(map). It's crazy enough to be worth explaining, I think.

@@ +390,5 @@
>  };
>  
>  static JSFunctionSpec weak_map_methods[] = {
>      JS_FN("has",    WeakMap_has, 1, 0),
> +    JS_FN("clear",  WeakMap_clear, 0, 0),

The order doesn't matter at all, but I wonder why "clear" fits in between "has" and "get" (the two non-mutating methods). I guess I would put it last--but it's fine where it is.
Attachment #687906 - Flags: review?(jorendorff) → review+
Comment on attachment 687908 [details] [diff] [review]
Test v1

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

::: js/src/jit-test/tests/collections/WeakMap-clear.js
@@ +13,5 @@
> +assertEq(wm.has(key), false);
> +
> +// Clearing a WeakMap with keys turned to garbage
> +for (var i = 0; i < 10; i++)
> +    wm.set(key, {});

This key never becomes garbage. It is directly referenced from a global variable. Instead, use:

    wm.set(key, {});
    for (var i = 0; i < 10; i++)
        wm.set({}, {});  // these keys become garbage immediately

@@ +20,5 @@
> +assertEq(wm.has(key), false);
> +
> +// Clearing a WeakMap with keys turned to garbage and after doing a GC
> +for (var i = 0; i < 10; i++)
> +    wm.set(key, {});

Same thing here.
Attachment #687908 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/b5fa6a857c35
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 1101817
You need to log in before you can comment on or make changes to this bug.