Implement ES6 Set class
Created attachment 210163 [details] Patch
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?
(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 on attachment 210163 [details] Patch Please also add benchmarks to js/regress. Make them run for about ~10-20 ms on a decent Mac.
Created attachment 210171 [details] Patch
Comment on attachment 210171 [details] Patch I like it.
Committed r154916: <http://trac.webkit.org/changeset/154916>