Bug 67299 - Add Android's platform specification and atomic functions
Summary: Add Android's platform specification and atomic functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66687 66689
  Show dependency treegraph
 
Reported: 2011-08-31 10:42 PDT by Peter Beverloo
Modified: 2011-08-31 14:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.40 KB, patch)
2011-08-31 10:43 PDT, Peter Beverloo
no flags Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2011-08-31 11:39 PDT, Peter Beverloo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2011-08-31 10:42:11 PDT
This will re-add the Platform.h changes removed in r94191, and adds Android's atomic functions to WTF.
Comment 1 Peter Beverloo 2011-08-31 10:43:15 PDT
Created attachment 105790 [details]
Patch
Comment 2 Steve Block 2011-08-31 10:59:37 PDT
Comment on attachment 105790 [details]
Patch

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

LGTM, but I'd like to get somebody else to take a look to double-check the change to Platform.h, perhaps Maciej?

Also, shouldn't this bug block Bug 66689?

> Source/JavaScriptCore/wtf/Atomics.h:109
> +inline int atomicDecrement(int volatile* addend) { return __atomic_dec(addend); }

So __atomic_inc/dec() are functionally identical to android_atomic_inc/dec() removed in r94191, but are in the NDK, right?

> Source/JavaScriptCore/wtf/Platform.h:306
> +/* OS(ANDRO67299ID) - Android */

??!!
Comment 3 Peter Beverloo 2011-08-31 11:39:56 PDT
Created attachment 105805 [details]
Patch
Comment 4 Peter Beverloo 2011-08-31 11:41:50 PDT
(In reply to comment #2)
> (From update of attachment 105790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105790&action=review
> 
> LGTM, but I'd like to get somebody else to take a look to double-check the change to Platform.h, perhaps Maciej?

Ok, thanks.

> 
> Also, shouldn't this bug block Bug 66689?

Yes, you're right. I've added the bug, but won't remove the main one as that'll unnecessarily mail all cc'ed people again.

> 
> > Source/JavaScriptCore/wtf/Atomics.h:109
> > +inline int atomicDecrement(int volatile* addend) { return __atomic_dec(addend); }
> 
> So __atomic_inc/dec() are functionally identical to android_atomic_inc/dec() removed in r94191, but are in the NDK, right?

Yes.

> 
> > Source/JavaScriptCore/wtf/Platform.h:306
> > +/* OS(ANDRO67299ID) - Android */
> 
> ??!!

Ugh, fixed.
Comment 5 Adam Barth 2011-08-31 13:54:07 PDT
Comment on attachment 105805 [details]
Patch

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

The Platform.h changes look fine to me.

> Source/JavaScriptCore/wtf/Platform.h:698
>  #if !OS(WINDOWS) && !OS(SOLARIS) && !OS(QNX) \
>      && !OS(SYMBIAN) && !OS(HAIKU) && !OS(RVCT) \
> -    && !PLATFORM(BREWMP)
> +    && !OS(ANDROID) && !PLATFORM(BREWMP)

This should probably be a whitelist, but that's a change for another day.
Comment 6 WebKit Review Bot 2011-08-31 14:47:21 PDT
Comment on attachment 105805 [details]
Patch

Clearing flags on attachment: 105805

Committed r94235: <http://trac.webkit.org/changeset/94235>
Comment 7 WebKit Review Bot 2011-08-31 14:47:26 PDT
All reviewed patches have been landed.  Closing bug.