Bug 211655 – Fix build errors and warnings for non-unified JSCOnly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Fix build errors and warnings for non-unified JSCOnly

[Ross Kirsling]

Created attachment 398908 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 398908 [details]
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.

[Ross Kirsling]

Created attachment 398911 [details]
Patch

[Ross Kirsling]

Those were precisely the cases that I wanted review feedback on, thanks. :)

[Darin Adler]

Comment on attachment 398911 [details]
Patch

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

> Source/JavaScriptCore/tools/JSDollarVM.cpp:328
> +IGNORE_WARNINGS_BEGIN("unused-const-variable")

The need for this is unfortunate. Would be nice to find a way to avoid it.

[Ross Kirsling]

(In reply to Darin Adler from comment #5)
> Comment on attachment 398911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398911&action=review
> 
> > Source/JavaScriptCore/tools/JSDollarVM.cpp:328
> > +IGNORE_WARNINGS_BEGIN("unused-const-variable")
> 
> The need for this is unfortunate. Would be nice to find a way to avoid it.

I do agree; it seems like a comment would be more than sufficient, since there are about 17 JSCell-inheriting class declarations in this file and the only two actually overriding needsDestruction are doing so implicitly by deriving from JSDestructibleObject. Not sure why two of the others should need to be explicit about choosing not to override.

[Ross Kirsling]

Created attachment 398942 [details]
Patch for landing

[EWS]

Committed r261441: <https://trac.webkit.org/changeset/261441>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398942 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63057435>