Bug 155438 – Need to distinguish between Symbol() and Symbol("").

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

While toString of both Symbol() and Symbol("") yields "Symbol()", Function.name should yield "" and "[]" respectively.  Hence, we need to tell between the two.

[Mark Lam]

Created attachment 273973 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 273973 [details]
proposed patch.

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

Can we use some mechanism other than a new flag to indicate that the Symbol is null? How about setting m_data8/m_data16 to nullptr?

> Source/WTF/wtf/text/StringImpl.h:305
> +        CreateEmptySymbol = StringEmptySymbol

There's no need to make this enumeration equal another enumeration. This would be clearer without the =.

In String and StringImpl, we call String("") "empty" and we call String() "null".

Therefore, this new kind of Symbol is the *null* Symbol, not the empty Symbol.

[Mark Lam]

Created attachment 274034 [details]
proposed patch applying Geoff's feedback.

(In reply to comment #2)
> Can we use some mechanism other than a new flag to indicate that the Symbol
> is null? How about setting m_data8/m_data16 to nullptr?

There are lot of assertions in StringImpl that suggests that it is an expected invariant that m_data8 / m_data16 are never null.  That said, there is a better implementation that I can use that does not require changing the flags i.e. to use a singleton null StringImpl.

> > Source/WTF/wtf/text/StringImpl.h:305
> > +        CreateEmptySymbol = StringEmptySymbol
> 
> There's no need to make this enumeration equal another enumeration. This
> would be clearer without the =.

Removed in the new patch.

> In String and StringImpl, we call String("") "empty" and we call String()
> "null".
> 
> Therefore, this new kind of Symbol is the *null* Symbol, not the empty
> Symbol.

"Symbol()" being called "empty" preceded this patch.  My new patch will rename SymbolEmpty to be a NullSymbol.

[Saam Barati]

Comment on attachment 274034 [details]
proposed patch applying Geoff's feedback.

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

> Source/WTF/wtf/text/StringStatics.cpp:46
> +    static NeverDestroyed<StringImpl>nullString(ConstructEmptyString);

style: space between ">" and "n"

[Mark Lam]

Thanks for the review.

The mac test failure was pre-existing: See https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK1%20%28Tests%29/builds/3689.

I'll land shortly.

[Mark Lam]

Landed in r198168: <http://trac.webkit.org/r198168>.