Bug 137807 – Move JSC::MacroAssemblerX86Common::s_sse2CheckState definition to the right place

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Milan Crha]

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.

[Milan Crha]

Created attachment 239999 [details]
proposed patch

Just move the variable definition, as noted above.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

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

[Milan Crha]

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.

[Geoffrey Garen]

Comment on attachment 239999 [details]
proposed patch

I agree: Best to use X86Common.

[Milan Crha]

Created attachment 240101 [details]
proposed patch ][

Move the chunk into MacroAssemblerX86Common.cpp

[Csaba Osztrogonác]

(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

[Csaba Osztrogonác]

Created attachment 240111 [details]
patch for EWS

checking if the original patch + bug137873 make Windows EWS happy

[Csaba Osztrogonác]

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.

[Milan Crha]

Created attachment 240138 [details]
proposed patch ]I[

Version ][ with added ChangeLog entry.

[Csaba Osztrogonác]

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?)

[WebKit Commit Bot]

Comment on attachment 240138 [details]
proposed patch ]I[

Clearing flags on attachment: 240138

Committed r174933: <http://trac.webkit.org/changeset/174933>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Milan Crha]

(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).

[Csaba Osztrogonác]

(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