Bug 114779 – Use StringJoiner to create the JSString of arrayProtoFuncToString

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

[Benjamin Poulain]

Use StringJoiner to create the JSString of arrayProtoFuncToString

[Benjamin Poulain]

Created attachment 198668 [details]
Patch

[Benjamin Poulain]

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()

[Geoffrey Garen]

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.

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

(JSObject::getIndexQuickly() cannot throw.)

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 198763 [details]
Patch

[Benjamin Poulain]

Comment on attachment 198763 [details]
Patch

Clearing flags on attachment: 198763

Committed r148721: <http://trac.webkit.org/changeset/148721>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.