RESOLVED FIXED 195962
Remove copyRef() calls added in r243163
https://bugs.webkit.org/show_bug.cgi?id=195962
Summary Remove copyRef() calls added in r243163
Michael Catanzaro
Reported 2019-03-19 13:56:56 PDT
I added some calls to copyRef() in r243163 that Chris wants removed, see bug #195920.
Attachments
Patch (4.12 KB, patch)
2019-03-19 15:34 PDT, Michael Catanzaro
no flags
Archive of layout-test-results from ews202 for win-future (12.92 MB, application/zip)
2019-03-19 19:00 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.04 MB, application/zip)
2019-03-19 19:58 PDT, EWS Watchlist
no flags
Michael Catanzaro
Comment 1 2019-03-19 15:34:06 PDT
EWS Watchlist
Comment 2 2019-03-19 15:36:56 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
EWS Watchlist
Comment 3 2019-03-19 19:00:31 PDT
Comment on attachment 365245 [details] Patch Attachment 365245 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11573478 New failing tests: js/dom/custom-constructors.html
EWS Watchlist
Comment 4 2019-03-19 19:00:42 PDT
Created attachment 365286 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
EWS Watchlist
Comment 5 2019-03-19 19:58:35 PDT
Comment on attachment 365245 [details] Patch Attachment 365245 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11574224 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
EWS Watchlist
Comment 6 2019-03-19 19:58:37 PDT
Created attachment 365297 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Chris Dumez
Comment 7 2019-03-20 09:13:43 PDT
Comment on attachment 365245 [details] Patch Clearing flags on attachment: 365245 Committed r243204: <https://trac.webkit.org/changeset/243204>
Chris Dumez
Comment 8 2019-03-20 09:13:45 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2019-03-20 09:14:46 PDT
Darin Adler
Comment 10 2019-03-20 09:55:05 PDT
Comment on attachment 365245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365245&action=review > Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:235 > + Ref<JSON::Object> jsonResult = m_result.releaseNonNull(); > + auto result = WTFMove(*reinterpret_cast<Ref<${objectType}>*>(&jsonResult)); > + return result; I would think we’d need to add a comment explaining that this is to work around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the local variable. Another way to protect against that is to make sure we have at least EWS testing completion with GCC 9.
Michael Catanzaro
Comment 11 2019-03-20 10:09:13 PDT
(In reply to Darin Adler from comment #10) > I would think we’d need to add a comment explaining that this is to work > around a GCC 9 bug. Otherwise, someone could refactor this to get rid of the > local variable. Another way to protect against that is to make sure we have > at least EWS testing completion with GCC 9. I almost added a comment, but in the end I decided it was low-value since if it's ever refactored, we'll either get a warning again and know right away, or else we won't (if GCC stops warning there) and it will be fine. EWS won't catch it unless we enable -Werror. There are pros and cons to enabling -Werror on the EWS. The risk is that it will probably significantly increase the number of build failures. The benefit is that it will surely reduce the number of compiler warnings introduced. (I agree it would be nice to have EWS testing more supported versions of GCC.)
Note You need to log in before you can comment on or make changes to this bug.