Bug 120549 – Implement ES6 Set class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Implement ES6 Set class

[Oliver Hunt]

Created attachment 210163 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 210163 [details]
Patch

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

Looks good, just some specific questions due to my ignorance.

> Source/JavaScriptCore/runtime/SetConstructor.cpp:55
> +        return throwVMError(callFrame, createRangeError(callFrame, WTF::ASCIILiteral("Invalid comparator function")));

Would it make sense to also say "Set constructor does not accept arguments" here too to avoid confusion from anybody trying this feature out? Also, this is in the case where they say "Set()" rather than "new Set()", correct? Does it make sense to call this a "constructor" at all in that case?

> Source/JavaScriptCore/runtime/SetPrototype.cpp:88
> +    if (!data)
> +        return JSValue::encode(jsUndefined());

Should we crash here? Not having a MapData seems like it should never happen, no?

> Source/JavaScriptCore/runtime/SetPrototype.cpp:120
> +    JSValue thisValue = callFrame->argument(1);

What happens if they don't pass a second argument? Should thisValue be undefined or the global object or what?

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

What happens if we get an exception? Should we return it like we did for the no-callback case?

[Oliver Hunt]

(In reply to comment #2)
> (From update of attachment 210163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210163&action=review
> 
> Looks good, just some specific questions due to my ignorance.
> 
> > Source/JavaScriptCore/runtime/SetConstructor.cpp:55
> > +        return throwVMError(callFrame, createRangeError(callFrame, WTF::ASCIILiteral("Invalid comparator function")));
Made better message

> 
> Would it make sense to also say "Set constructor does not accept arguments" here too to avoid confusion from anybody trying this feature out? Also, this is in the case where they say "Set()" rather than "new Set()", correct? Does it make sense to call this a "constructor" at all in that case?
> 
> > Source/JavaScriptCore/runtime/SetPrototype.cpp:88
> > +    if (!data)
> > +        return JSValue::encode(jsUndefined());
> 
> Should we crash here? Not having a MapData seems like it should never happen, no?

these are prototype functions, you can call them on anything you would like (e.g. someSet.map.call({}) )

> > Source/JavaScriptCore/runtime/SetPrototype.cpp:120
> > +    JSValue thisValue = callFrame->argument(1);
> 
> What happens if they don't pass a second argument? Should thisValue be undefined or the global object or what?
> 
> > Source/JavaScriptCore/runtime/SetPrototype.cpp:137
> > +    return JSValue::encode(jsUndefined());
> 
> What happens if we get an exception? Should we return it like we did for the no-callback case?

Nope, we just return exceptions/value to guard against other code failing to check for an exception before trying to use something as a value.

[Filip Pizlo]

Comment on attachment 210163 [details]
Patch

Please also add benchmarks to js/regress.  Make them run for about ~10-20 ms on a decent Mac.

[Oliver Hunt]

Created attachment 210171 [details]
Patch

[Filip Pizlo]

Comment on attachment 210171 [details]
Patch

I like it.

[Oliver Hunt]

Committed r154916: <http://trac.webkit.org/changeset/154916>