[Webkit-unassigned] [Bug 216172] [JSC] Use symbols as identifiers for class fields computed names storage

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=216172

--- Comment #24 from Xan Lopez <xan.lopez at gmail.com> ---
(In reply to Caio Lima from comment #22)
> Comment on attachment 412513 [details]
> Use symbols as identifiers, v8
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412513&action=review
> 
> Patch LGTM. I left a comment below.
> 
> > Source/WTF/wtf/text/SymbolRegistry.h:94
> > +    CreateSymbol m_createSymbol { nullptr };
> 
> I'm wondering if it is really worth it have those 2 new members on
> `SymbolRegistry`. I'm neutral on `m_symbolType` but I'm not sold on
> `m_createSymbol`, since we could call either
> `RegisteredSymbolImpl::createPrivate` or `RegisteredSymbolImpl::create`
> directly on `symbolForKey`  based on `m_symbolType`.

Yeah, I think a previous version did this. If memory is a concern we could do it again (also the opposite, just set the function once in the ctor and then forget the type).

About the test, there is one in test262 for static fields that verifies this works, but it won't land with this patch. We should add a test262 test for instance fields too, but it also would not land with this patch, right? I'd say doing that is enough coverage, but I could add a test for it locally if you think that's important.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201106/135cae0b/attachment.htm>