[webkit-reviews] review granted: [Bug 185637] UnlinkedFunctionExecutable doesn't need a parent source override field since it's only used for default class constructors : [Attachment 340444] patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Saam Barati
<sbarati at apple.com>'s request for review:
Bug 185637: UnlinkedFunctionExecutable doesn't need a parent source override
field since it's only used for default class constructors
https://bugs.webkit.org/show_bug.cgi?id=185637

Attachment 340444: patch

https://bugs.webkit.org/attachment.cgi?id=340444&action=review




--- Comment #4 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 340444
  --> https://bugs.webkit.org/attachment.cgi?id=340444
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340444&action=review

r=me. Maybe it's worth adding a
static_assert(sizeof(UnlinkedFunctionExecutable) <= 160, "Increasing the size
of UnlinkedFunctionExecutable means that it increases its size class");?

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:66
> +    case ConstructorKind::Extends:
> +	   static NeverDestroyed<const String>
derivedConstructorCode(MAKE_STATIC_STRING_IMPL("(function (...args) {
super(...args); })"));
> +	   static LazyNeverDestroyed<SourceCode> result;
> +	   static std::once_flag onceFlag;
> +	   std::call_once(onceFlag, [&] {
> +	       result.construct(makeSource(derivedConstructorCode, { }));
> +	   });
> +	   return result;

Nit: put { } around the case since you are allocating variables.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:77
> -	   return createExecutable(m_vm, makeSource(baseConstructorCode, { }),
name, constructorKind, ConstructAbility::CanConstruct);
> +	   return createExecutable(m_vm,
defaultConstructorSourceCode(constructorKind), name, constructorKind,
ConstructAbility::CanConstruct);

I think you can get rid of this line.