While toString of both Symbol() and Symbol("") yields "Symbol()", Function.name should yield "" and "[]" respectively. Hence, we need to tell between the two.
Created attachment 273973 [details] proposed patch.
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.
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.
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"
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.
Landed in r198168: <http://trac.webkit.org/r198168>.