| Summary: | Move JSC::MacroAssemblerX86Common::s_sse2CheckState definition to the right place | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Milan Crha <mcrha> | ||||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | barraclough, commit-queue, oliver, ossy | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 137873 | ||||||||||||
| Bug Blocks: | 137488 | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 239999 [details]
proposed patch
Just move the variable definition, as noted above.
It was originally introduced in https://trac.webkit.org/changeset/43789. cc-ing Gavin as the author and Oliver as the reviewer of the patch. Comment on attachment 239999 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=239999&action=review > Source/JavaScriptCore/assembler/MacroAssembler.cpp:38 > +#if CPU(X86) && !OS(MAC_OS_X) > +MacroAssemblerX86Common::SSE2CheckState MacroAssemblerX86Common::s_sse2CheckState = NotCheckedSSE2; > +#endif > + Why don't we move it into MacroAssemblerX86Common.cpp? It is only used inside MacroAssemblerX86Common.h You are right, that's even better place. I didn't notice it being compiled on my machine (overlooked it), I noticed the MacroAssembler.cpp only, thus picked it. Comment on attachment 239999 [details]
proposed patch
I agree: Best to use X86Common.
Created attachment 240101 [details]
proposed patch ][
Move the chunk into MacroAssemblerX86Common.cpp
(In reply to comment #6) > Created attachment 240101 [details] > proposed patch ][ > > Move the chunk into MacroAssemblerX86Common.cpp The Windows EWS is unhappy. It seems MacroAssemblerX86Common.cpp isn't built on Windows at all due to a typo here: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj#L308 I filed a new bug report to make this source build on Windows too: bug137873 Created attachment 240111 [details] patch for EWS checking if the original patch + bug137873 make Windows EWS happy Comment on attachment 240101 [details] proposed patch ][ r- now due to missing changelog. Please add a changelog entry with Tools/Scripts/prepare-changelog and upload the patch once the fix in bug137873 landed. Created attachment 240138 [details]
proposed patch ]I[
Version ][ with added ChangeLog entry.
Comment on attachment 240138 [details] proposed patch ]I[ View in context: https://bugs.webkit.org/attachment.cgi?id=240138&action=review r=me > Source/JavaScriptCore/ChangeLog:6 > + Reviewed by Csaba Osztrogonác. You shouldn't have add my name manually here, commit queue automatically relaces ooops before landing. (And what if anyone else would like to review the patch?) Comment on attachment 240138 [details] proposed patch ]I[ Clearing flags on attachment: 240138 Committed r174933: <http://trac.webkit.org/changeset/174933> All reviewed patches have been landed. Closing bug. (In reply to comment #11) > You shouldn't have add my name manually here, > commit queue automatically relaces ooops before landing. > > (And what if anyone else would like to review the patch?) That was just a wild guess. I'm new in the WebKit commit process, my first ChangeLog entry I can remember. May I keep all three OOPS unchanged there? One was for a short description, another one for a bug link (the third was the aforementioned reviewer). (In reply to comment #14) > (In reply to comment #11) > > You shouldn't have add my name manually here, > > commit queue automatically relaces ooops before landing. > > > > (And what if anyone else would like to review the patch?) > > That was just a wild guess. I'm new in the WebKit commit process, my first > ChangeLog entry I can remember. > > May I keep all three OOPS unchanged there? One was for a short description, > another one for a bug link (the third was the aforementioned reviewer). You can find the details here: http://www.webkit.org/coding/contributing.html |
The Source/JavaScriptCore/jit/JIT.cpp defines JSC::MacroAssemblerX86Common::s_sse2CheckState with a comment: > // This probably does not belong here; adding here for now as a quick Windows build fix. and it really is not, because with disabled JIT the variable is not defined and the build breaks. The right (or better place) for it is MacroAssembler.cpp, just beside another static variable definition.