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
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)".
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 :)
Landed in r153345 (http://trac.webkit.org/changeset/153345).
(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 ;)