| Summary: | Need to distinguish between Symbol() and Symbol(""). | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, fpizlo, ggaren, keith_miller, msaboff, saam, ysuzuki | ||||||
| Priority: | P2 | ||||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 155437 | ||||||||
| Attachments: |
|
||||||||
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>. |
While toString of both Symbol() and Symbol("") yields "Symbol()", Function.name should yield "" and "[]" respectively. Hence, we need to tell between the two.