Bug 170210

Summary: LinkBuffer and ExecutableAllocator shouldn't have anything to do with VM
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, ticaiolima, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 168264    
Attachments:
Description Flags
WIP
none
WIP
none
WIP
none
WIP
none
patch
mark.lam: review+
patch for landing none

Saam Barati
Reported 2017-03-28 15:51:40 PDT
...
Attachments
WIP (37.04 KB, patch)
2017-03-28 19:42 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
WIP (115.87 KB, patch)
2017-03-29 13:36 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
WIP (117.74 KB, patch)
2017-03-29 13:38 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
WIP (121.47 KB, patch)
2017-03-29 14:05 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
patch (134.26 KB, patch)
2017-03-29 14:17 PDT, Saam Barati 		mark.lam: review+
DetailsFormatted DiffDiff
patch for landing (134.31 KB, patch)
2017-03-29 14:33 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2017-03-28 19:42:23 PDT
Created attachment 305695 [details] WIP
Saam Barati
Comment 2 2017-03-29 13:36:42 PDT
Created attachment 305779 [details] WIP lets see what EWS thinks
Saam Barati
Comment 3 2017-03-29 13:38:35 PDT
Created attachment 305780 [details] WIP added webcore change
Saam Barati
Comment 4 2017-03-29 14:05:08 PDT
Created attachment 305783 [details] WIP fixed some bad lambda capture
Saam Barati
Comment 5 2017-03-29 14:17:14 PDT
Created attachment 305788 [details] patch
Build Bot
Comment 6 2017-03-29 14:20:25 PDT
Attachment 305788 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Lam
Comment 7 2017-03-29 14:28:13 PDT
Comment on attachment 305788 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=305788&action=review r=me with suggestions. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5984 > + VM* vm = &this->vm(); nit: can we use a VM& instead? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6282 > + VM* vm = &this->vm(); Ditto. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6408 > + VM* vm = &this->vm(); Ditto. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6678 > + VM* vm = &this->vm(); Ditto. > Source/JavaScriptCore/jit/ExecutableAllocator.h:141 > +private: > + nit: can you put the empty line before the private: section instead of after? > Source/JavaScriptCore/runtime/VM.cpp:169 > #if ENABLE(ASSEMBLER) > - , executableAllocator(*this) > #endif Remove these lines altogether.
Saam Barati
Comment 8 2017-03-29 14:32:19 PDT
Comment on attachment 305788 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=305788&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5984 >> + VM* vm = &this->vm(); > > nit: can we use a VM& instead? The reason I did this is that changing to VM& would require 2 more lines of code. And it's also a bit more in line with the style in FTLLower to get a pointer. In principle, I agree, though. We know the thing will never be null.
Saam Barati
Comment 9 2017-03-29 14:33:10 PDT
Created attachment 305794 [details] patch for landing
WebKit Commit Bot
Comment 10 2017-03-29 15:55:57 PDT
Comment on attachment 305794 [details] patch for landing Clearing flags on attachment: 305794 Committed r214571: <http://trac.webkit.org/changeset/214571>
WebKit Commit Bot
Comment 11 2017-03-29 15:56:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.