Summary: | Fix build errors and warnings for non-unified JSCOnly | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||
Component: | New Bugs | Assignee: | Ross Kirsling <ross.kirsling> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ross Kirsling
2020-05-08 18:08:41 PDT
Created attachment 398908 [details]
Patch
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. Created attachment 398911 [details]
Patch
Those were precisely the cases that I wanted review feedback on, thanks. :) 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. (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. Created attachment 398942 [details]
Patch for landing
Committed r261441: <https://trac.webkit.org/changeset/261441> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398942 [details]. |