Bug 143424 - [ES6] DFG and FTL should be aware of that StringConstructor behavior for symbols becomes different from ToString
Summary: [ES6] DFG and FTL should be aware of that StringConstructor behavior for symb...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
: 143427 (view as bug list)
Depends on:
Blocks: 141279
  Show dependency treegraph
 
Reported: 2015-04-05 14:49 PDT by Yusuke Suzuki
Modified: 2015-04-06 12:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch (21.77 KB, patch)
2015-04-05 15:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (21.95 KB, patch)
2015-04-05 18:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-04-05 14:49:45 PDT
In ES6, StringConstructor behavior becomes different from ToString abstract operations in the spec. (and JSValue::toString).

ToString(symbol) throws a type error.
String(symbol) produces  SymbolDescriptiveString(value).

So, in DFG and FTL phase, they should not inline String constructor to ToString.
Now, in the template literals patch, ToString DFG operation is planned to be used for it.
And current ToString behavior is aligned to the spec (and JSValue::toString). I think keeping it is better.
So intead of changing ToString behavior, I think adding StringConstructor operation into DFG and FTL is nice.
Comment 1 Yusuke Suzuki 2015-04-05 15:49:11 PDT
Created attachment 250171 [details]
Patch
Comment 2 Yusuke Suzuki 2015-04-05 17:42:44 PDT
*** Bug 143427 has been marked as a duplicate of this bug. ***
Comment 3 Joseph Pecoraro 2015-04-05 17:46:40 PDT
Comment on attachment 250171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250171&action=review

> Source/JavaScriptCore/runtime/StringConstructor.cpp:112
> +        return jsString(exec, asSymbol(argument)->descriptiveString());

Nit: This can be jsNontrivialString, which can be used for Strings guaranteed to be 2 or more characters long, and this is guaranteed to have "Symbol()".
Comment 4 Yusuke Suzuki 2015-04-05 18:01:28 PDT
Comment on attachment 250171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250171&action=review

>> Source/JavaScriptCore/runtime/StringConstructor.cpp:112
>> +        return jsString(exec, asSymbol(argument)->descriptiveString());
> 
> Nit: This can be jsNontrivialString, which can be used for Strings guaranteed to be 2 or more characters long, and this is guaranteed to have "Symbol()".

Thank you! I've changed it.
Comment 5 Yusuke Suzuki 2015-04-05 18:04:10 PDT
Created attachment 250182 [details]
Patch
Comment 6 Geoffrey Garen 2015-04-06 11:18:27 PDT
Comment on attachment 250182 [details]
Patch

r=me
Comment 7 Yusuke Suzuki 2015-04-06 11:19:42 PDT
Comment on attachment 250182 [details]
Patch

Thank you for your review :D
Comment 8 WebKit Commit Bot 2015-04-06 12:08:06 PDT
Comment on attachment 250182 [details]
Patch

Clearing flags on attachment: 250182

Committed r182433: <http://trac.webkit.org/changeset/182433>
Comment 9 WebKit Commit Bot 2015-04-06 12:08:10 PDT
All reviewed patches have been landed.  Closing bug.