[webkit-reviews] review denied: [Bug 211655] Fix build errors and warnings for non-unified JSCOnly : [Attachment 398908] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 211655: Fix build errors and warnings for non-unified JSCOnly
https://bugs.webkit.org/show_bug.cgi?id=211655

Attachment 398908: Patch

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 398908
  --> https://bugs.webkit.org/attachment.cgi?id=398908
Patch

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

Nice, but I think some of warning should be disabled.

> Source/JavaScriptCore/ChangeLog:16
> +	   Move function definition to avoid needing to include VM.h from
header.

I think we should keep it inline since it is one of the important function, and
vmEntryToJavaScript is always non-inlined LLInt code. So this wrapper should be
inline.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:-327
> -    static constexpr bool needsDestruction = false;

It should be defined here to make SimpleObject non-destruction explicit for
readability. JS Cell object hierarchy should be carefully managed in particular
if they are using `cellSpace`.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:-564
> -    static constexpr bool needsDestruction = false;
> -

It should be defined here to make RuntimeArray non-destruction explicit for
readability.