[webkit-reviews] review granted: [Bug 195508] [JSC] BuiltinExecutables should behave like a WeakSet instead of generic WeakHandleOwner for memory footprint : [Attachment 364111] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 195508: [JSC] BuiltinExecutables should behave like a WeakSet instead of
generic WeakHandleOwner for memory footprint
https://bugs.webkit.org/show_bug.cgi?id=195508

Attachment 364111: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 364111
  --> https://bugs.webkit.org/attachment.cgi?id=364111
Patch

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

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:264
> +    for (UnlinkedFunctionExecutable*& executable : m_executables) {

I know other people don’t agree, but I really prefer "auto&" in a case like
this.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:273
> +    return SourceCode(m_combinedSourceProvider.copyRef(), s_##name -
s_JSCCombinedCode, (s_##name - s_JSCCombinedCode) + length, 1, 1);\

The SourceCode { } syntax might be better to use here, giving slightly stricter
type checking. And possibly just { } without the explicit SourceCode.

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:40
> +#define PUT_BUILTIN_CODE_ID(name, functionName, overriddenName, length)
name,

Do we want to also include an #undef for this one? Maybe a shorter name for the
macro too, like BUILTIN_NAME_ONLY.

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:64
> +    void finalizeUnconditionally();
> +private:

Typically we leave a blank line before the private section.