Bug 114779

Summary: Use StringJoiner to create the JSString of arrayProtoFuncToString
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Benjamin Poulain 2013-04-17 19:02:58 PDT
Use StringJoiner to create the JSString of arrayProtoFuncToString
Comment 1 Benjamin Poulain 2013-04-17 19:05:55 PDT
Created attachment 198668 [details]
Patch
Comment 2 Benjamin Poulain 2013-04-17 19:11:22 PDT
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 3 Geoffrey Garen 2013-04-17 19:14:45 PDT
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.
Comment 4 Geoffrey Garen 2013-04-17 19:15:49 PDT
> 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.
Comment 5 Geoffrey Garen 2013-04-17 19:16:53 PDT
(JSObject::getIndexQuickly() cannot throw.)
Comment 6 Benjamin Poulain 2013-04-17 20:35:06 PDT
(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.
Comment 7 Benjamin Poulain 2013-04-18 14:37:52 PDT
Created attachment 198763 [details]
Patch
Comment 8 Benjamin Poulain 2013-04-18 18:51:29 PDT
Comment on attachment 198763 [details]
Patch

Clearing flags on attachment: 198763

Committed r148721: <http://trac.webkit.org/changeset/148721>
Comment 9 Benjamin Poulain 2013-04-18 18:51:33 PDT
All reviewed patches have been landed.  Closing bug.