Bug 120549 - Implement ES6 Set class
Summary: Implement ES6 Set class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-30 14:34 PDT by Oliver Hunt
Modified: 2013-08-30 15:54 PDT (History)
5 users (show)

See Also:


Attachments
Patch (55.96 KB, patch)
2013-08-30 14:46 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (57.83 KB, patch)
2013-08-30 15:48 PDT, Oliver Hunt
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-08-30 14:34:03 PDT
Implement ES6 Set class
Comment 1 Oliver Hunt 2013-08-30 14:46:26 PDT
Created attachment 210163 [details]
Patch
Comment 2 Mark Hahnenberg 2013-08-30 15:19:48 PDT
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?
Comment 3 Oliver Hunt 2013-08-30 15:24:39 PDT
(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.
Comment 4 Filip Pizlo 2013-08-30 15:26:40 PDT
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.
Comment 5 Oliver Hunt 2013-08-30 15:48:00 PDT
Created attachment 210171 [details]
Patch
Comment 6 Filip Pizlo 2013-08-30 15:52:45 PDT
Comment on attachment 210171 [details]
Patch

I like it.
Comment 7 Oliver Hunt 2013-08-30 15:54:25 PDT
Committed r154916: <http://trac.webkit.org/changeset/154916>