[webkit-reviews] review granted: [Bug 65422] Simplify JSFunction creation for functions written in JS : [Attachment 102447] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 65422: Simplify JSFunction creation for functions written in JS
https://bugs.webkit.org/show_bug.cgi?id=65422

Attachment 102447: Patch
https://bugs.webkit.org/attachment.cgi?id=102447&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=102447&action=review


> Source/JavaScriptCore/runtime/Executable.cpp:147
> +    m_jsName.set(globalData, name.isEmpty() ?
globalData.smallStrings.emptyString(&globalData) : jsString(&globalData,
name.ustring()));

Can this be done with construction instead of assignment? Would that be a tiny
bit better for performance?

This expression is long enough that it might be nice to share this code with an
inline function instead of repeating it twice. The easiest way to do that would
be an inline member function, or we could use an inline non-member function if
we are doing it in construction.

Doesn’t jsString already handle the empty string case? Why do we need a special
case for it here?

> Source/JavaScriptCore/runtime/Executable.h:481
> +	   JSString* jsName() const { return m_jsName.get(); }

I would think of this as nameValue or wrappedName rather than jsName.