Closed Bug 1081978 Opened 10 years ago Closed 10 years ago

Refine WeakSet handling

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: evilpie, Assigned: evilpie)

Details

Attachments

(1 file)

Attached patch weaksetSplinter Review
Right now we never really do Step 4 in the different methods: "If S’s [[WeakSetData]] internal slot is undefined, then throw a TypeError exception."
This has the added benefit that WeakSet.prototype (which shouldn't actually be a WeakSet) gives a better error message for now.
Attachment #8504081 - Flags: review?(till)
Assignee: nobody → evilpies
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8504081 [details] [diff] [review]
weakset

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

r=me with feedback addressed. If you think the "return false instead of throwing" changes should go in another patch: r=me on that.

::: js/src/builtin/WeakSet.js
@@ +50,5 @@
>      var S = this;
>      if (!IsObject(S) || !IsWeakSet(S))
>          ThrowError(JSMSG_INCOMPATIBLE_PROTO, "WeakSet", "delete", typeof S);
>  
> +    // Step 4.

Please change this to "Steps 4,6."

@@ +59,3 @@
>      // Step 5.
>      if (!IsObject(value))
>          ThrowError(JSMSG_NOT_NONNULL_OBJECT);

Pre-existing, but this isn't correct: it should just return false.

@@ +70,5 @@
>      var S = this;
>      if (!IsObject(S) || !IsWeakSet(S))
>          ThrowError(JSMSG_INCOMPATIBLE_PROTO, "WeakSet", "has", typeof S);
>  
> +    // Step 4.

"Steps 4-5."

@@ +79,1 @@
>      // Step 5.

"Step 6."

@@ +79,3 @@
>      // Step 5.
>      if (!IsObject(value))
>          ThrowError(JSMSG_NOT_NONNULL_OBJECT);

Pre-existing, but this isn't correct: it should just return false.
Attachment #8504081 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/88b3e8b81de8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.