Use StringJoiner to create the JSString of arrayProtoFuncToString
Created attachment 198668 [details] Patch
Comment on attachment 198668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198668&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:322 > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); To reviewers: This part is unclear to me. Shouldn't the code also check hadException() after invoking JSArray->get()? The code of arrayProtoFuncToLocaleString checks exception twice: -after JSArray::get() -after JSValue.toWTFString()
Comment on attachment 198668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198668&action=review r=me > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:324 > + return JSValue::encode(stringJoiner.build(exec)); As Ghost Dog would say, it is bad when one thing becomes two. It's a pet peeve of mine when a class has a name like "string joiner" and its action function is a different name like "build". This would read better as "stringJoiner.join()". Oh well.
> To reviewers: > This part is unclear to me. Shouldn't the code also check hadException() after invoking JSArray->get()? The code of arrayProtoFuncToLocaleString checks exception twice: > -after JSArray::get() > -after JSValue.toWTFString() Both JSArray::get() and JSValue.toWTFString() can throw, so yes, both should check for exception.
(JSObject::getIndexQuickly() cannot throw.)
(In reply to comment #4) > > To reviewers: > > This part is unclear to me. Shouldn't the code also check hadException() after invoking JSArray->get()? The code of arrayProtoFuncToLocaleString checks exception twice: > > -after JSArray::get() > > -after JSValue.toWTFString() > > Both JSArray::get() and JSValue.toWTFString() can throw, so yes, both should check for exception. I thought so! Thanks for checking that and reviewing.
Created attachment 198763 [details] Patch
Comment on attachment 198763 [details] Patch Clearing flags on attachment: 198763 Committed r148721: <http://trac.webkit.org/changeset/148721>
All reviewed patches have been landed. Closing bug.