Bug 155438 - Need to distinguish between Symbol() and Symbol("").
Summary: Need to distinguish between Symbol() and Symbol("").
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 155437
  Show dependency treegraph
 
Reported: 2016-03-14 07:48 PDT by Mark Lam
Modified: 2016-03-14 16:20 PDT (History)
10 users (show)

See Also:


Attachments
proposed patch. (7.59 KB, patch)
2016-03-14 07:56 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch applying Geoff's feedback. (8.95 KB, patch)
2016-03-14 15:17 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-03-14 07:48:08 PDT
While toString of both Symbol() and Symbol("") yields "Symbol()", Function.name should yield "" and "[]" respectively.  Hence, we need to tell between the two.
Comment 1 Mark Lam 2016-03-14 07:56:35 PDT
Created attachment 273973 [details]
proposed patch.
Comment 2 Geoffrey Garen 2016-03-14 09:53:06 PDT
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.
Comment 3 Mark Lam 2016-03-14 15:17:47 PDT
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 4 Saam Barati 2016-03-14 15:48:29 PDT
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"
Comment 5 Mark Lam 2016-03-14 16:17:04 PDT
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.
Comment 6 Mark Lam 2016-03-14 16:20:47 PDT
Landed in r198168: <http://trac.webkit.org/r198168>.