Bug 119084 – [Win] Compile fix.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

While compiling JavaScriptCore, I get the following error:

C:\WebKit\WebKitBuild\Release_WinCairo\include\private\wtf/Atomics.h(283): error C2059: syntax error : ''

Created attachment 207453 [details]
Patch

Assembly code taken from http://stackoverflow.com/questions/5796459/is-there-an-8-bit-atomic-cas-cmpxchg-intrinsic-for-x64-in-visual-c

[Brent Fulgham]

Comment on attachment 207453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207453&action=review

I want olliej or ggaren to double-check this, but it seems fine to me.

Due to Visual Studio's decision to not support inline assembly in 64-bit builds, we'll also have to start adding external ASM files on Windows builds.  Maybe this code could should end up in such an external file.

> Source/WTF/wtf/Atomics.h:278
> +#if OS(WINDOWS)

Shouldn't this be inside the "#if ENABLE(COMPARE_AND_SWAP)" macro?  I wonder if this block shouldn't be a sibling of the existing "#if CPU(X86) || CPU(X86_64)".

[Brent Fulgham]

I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback.

Created attachment 207483 [details]
Patch

(In reply to comment #4)
> I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback.

Ok, didn't see this comment before I updated patch, thanks :)

[Brent Fulgham]

Landed in r153345 (http://trac.webkit.org/changeset/153345).

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #4)
> > I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback.
> 
> Ok, didn't see this comment before I updated patch, thanks :)

I changed the bug slightly to reflect that we weren't fixing the build, but were providing a nice, optimized ASM implementation.  Very nice!

Thanks for coming up with this fix.

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > I'm going to land this manually, to move it inside the existing method declaration leaving the C implementation (added after this patch was suggested) as a final fallback.
> > 
> > Ok, didn't see this comment before I updated patch, thanks :)
> 
> I changed the bug slightly to reflect that we weren't fixing the build, but were providing a nice, optimized ASM implementation.  Very nice!
> 
> Thanks for coming up with this fix.

Very good, thanks for reviewing ;)