Bug 142408 – [ES6] Implement WeakSet

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

During upgrade of Map/Set/WeakMap constructor interface, I've found that WeakSet implementation is missing.

[Yusuke Suzuki]

Since MapData change becomes a little bit large, at first, I'll create the separate patch to integrate MapData into JSMap / JSSet.
https://bugs.webkit.org/show_bug.cgi?id=142556

[Yusuke Suzuki]

OK, MapData is done.
So I'll perform the same thing on WeakMapData :D

[Yusuke Suzuki]

At first, implement ES6 WeakSet and after that, optimize WeakMapData.

[Yusuke Suzuki]

Since WeakMap / WeakSet doesn't have iterators, optimization is a little bit easier than MapData.

[Yusuke Suzuki]

Created attachment 251103 [details]
Patch

[Darin Adler]

Comment on attachment 251103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251103&action=review

> Source/JavaScriptCore/runtime/JSWeakSet.cpp:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Copyright is based on date of publication (the time we publish in the open source repository). So it should be 2015 unless this code was published in 2013.

> Source/JavaScriptCore/runtime/JSWeakSet.h:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Same issue with copyright date.

> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:88
>      if (!key.isObject())
> -        return JSValue::encode(throwTypeError(callFrame, WTF::ASCIILiteral("A WeakMap cannot have a non-object key")));
> +        return JSValue::encode(jsBoolean(false));
>      return JSValue::encode(jsBoolean(map->remove(asObject(key))));

Now that this is always a boolean, I think we can write it more cleanly:

    return JSValue::encode(jsBoolean(key.isObject() && map->remove(asObject(key)));

No need for the three-line version with the separate early return.

> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:98
> +        return JSValue::encode(jsUndefined());

Should this be undefined, or null? The change log comment mentions only "false".

> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:110
>      if (!key.isObject())
> -        return JSValue::encode(throwTypeError(callFrame, WTF::ASCIILiteral("A WeakMap cannot have a non-object key")));
> +        return JSValue::encode(jsBoolean(false));
>      return JSValue::encode(jsBoolean(map->contains(asObject(key))));

Now that this is always a boolean, I think we can write it more cleanly:

    return JSValue::encode(jsBoolean(key.isObject() && map->contains(asObject(key)));

No need for the three-line version with the separate early return.

> Source/JavaScriptCore/runtime/WeakSetConstructor.cpp:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Same issue with copyright.

> Source/JavaScriptCore/runtime/WeakSetConstructor.h:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Same issue with copyright.

> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Same issue with copyright.

> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:57
> +        return 0;

nullptr

> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:64
> +    return 0;

nullptr

> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:75
> +    JSValue key = callFrame->argument(0);
> +    if (!key.isObject())
> +        return JSValue::encode(jsBoolean(false));
> +    return JSValue::encode(jsBoolean(map->remove(asObject(key))));

Now that this is always a boolean, I think we can write it more cleanly:

    return JSValue::encode(jsBoolean(key.isObject() && map->remove(asObject(key)));

No need for the three-line version with the separate early return.

> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:86
> +    if (!key.isObject())
> +        return JSValue::encode(jsBoolean(false));
> +    return JSValue::encode(jsBoolean(map->contains(asObject(key))));

Now that this is always a boolean, I think we can write it more cleanly:

    return JSValue::encode(jsBoolean(key.isObject() && map->contains(asObject(key)));

No need for the three-line version with the separate early return.

> Source/JavaScriptCore/runtime/WeakSetPrototype.h:2
> + * Copyright (C) 2013 Apple, Inc. All rights reserved.

Same issue with copyright date.

[Yusuke Suzuki]

Comment on attachment 251103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251103&action=review

Thank you for your review and detailed comments!

>> Source/JavaScriptCore/runtime/JSWeakSet.cpp:2
>> + * Copyright (C) 2013 Apple, Inc. All rights reserved.
> 
> Copyright is based on date of publication (the time we publish in the open source repository). So it should be 2015 unless this code was published in 2013.

Thanks! I'll fix it.

>> Source/JavaScriptCore/runtime/JSWeakSet.h:2
>> + * Copyright (C) 2013 Apple, Inc. All rights reserved.
> 
> Same issue with copyright date.

Done

>> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:88
>>      return JSValue::encode(jsBoolean(map->remove(asObject(key))));
> 
> Now that this is always a boolean, I think we can write it more cleanly:
> 
>     return JSValue::encode(jsBoolean(key.isObject() && map->remove(asObject(key)));
> 
> No need for the three-line version with the separate early return.

OK. I'll follow it :)

>> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:98
>> +        return JSValue::encode(jsUndefined());
> 
> Should this be undefined, or null? The change log comment mentions only "false".

Oops! Thank you. The code is correct. We need to fix change log comment.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap.prototype.get

>> Source/JavaScriptCore/runtime/WeakMapPrototype.cpp:110
>>      return JSValue::encode(jsBoolean(map->contains(asObject(key))));
> 
> Now that this is always a boolean, I think we can write it more cleanly:
> 
>     return JSValue::encode(jsBoolean(key.isObject() && map->contains(asObject(key)));
> 
> No need for the three-line version with the separate early return.

Done.

>> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:57
>> +        return 0;
> 
> nullptr

Nice! I also fixed them in WeakMapPrototype.cpp.

>> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:64
>> +    return 0;
> 
> nullptr

Done.

>> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:75
>> +    return JSValue::encode(jsBoolean(map->remove(asObject(key))));
> 
> Now that this is always a boolean, I think we can write it more cleanly:
> 
>     return JSValue::encode(jsBoolean(key.isObject() && map->remove(asObject(key)));
> 
> No need for the three-line version with the separate early return.

Done.

>> Source/JavaScriptCore/runtime/WeakSetPrototype.cpp:86
>> +    return JSValue::encode(jsBoolean(map->contains(asObject(key))));
> 
> Now that this is always a boolean, I think we can write it more cleanly:
> 
>     return JSValue::encode(jsBoolean(key.isObject() && map->contains(asObject(key)));
> 
> No need for the three-line version with the separate early return.

Done.

[Yusuke Suzuki]

Committed r182994: <http://trac.webkit.org/changeset/182994>