We discussed this on IRC. It would be nice to be able to use Atomic<T> where T is an enum. See for example bug 876029 comment 20.

This uses the __is_enum(T) compiler intrinsic, which seems to be supported on all our target platforms. See: https://tbpl.mozilla.org/?tree=Try&rev=e2098937ac81

For some odd reason, this patch causes the following error with GCC. Clang and MSVC work fine. In file included from ../../dist/include/mozilla/Atomics.h:176:0, from ../../../mfbt/tests/TestAtomics.cpp:6: /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic: In function 'int main()': /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for '__atomic_compare_exchange' /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for '__atomic_compare_exchange' See: https://tbpl.mozilla.org/?tree=Try&rev=ba498905e72f GCC is able to compile TestAtomics.cpp with the following changes. I do not know if there exists a general solution for GCC (other than not defining MOZ_HAVE_CXX11_ATOMICS for it). Any ideas? @@ -128,16 +128,20 @@ MOZ_ASSERT(atomic == array1 + 3, "CAS should have changed atomic's value."); } +enum EnumType { + EnumType_0 = 0, + EnumType_1 = 1, + EnumType_2 = 2, + EnumType_3 = 3 +}; + template<MemoryOrdering Order> static void TestEnumWithOrdering() { - enum EnumType { - EnumType_0 = 0, - EnumType_1 = 1, - EnumType_2 = 2, - EnumType_3 = 3 - }; + std::atomic<EnumType> dummyStdAtomic; + EnumType expected = EnumType_0; + dummyStdAtomic.compare_exchange_strong(expected, EnumType_1); Atomic<EnumType, Order> atomic(EnumType_2); MOZ_ASSERT(atomic == EnumType_2, "Atomic variable did not initialize");

Comment on attachment 778943 [details] [diff] [review] Part 1: Add mozilla::IsEnum to TypeTraits.h Review of attachment 778943 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/TypeTraits.h @@ +129,5 @@ > +namespace detail { > + > +template<typename T> > +struct IsEnumHelper > + : IntegralConstant<bool, __is_enum(T)> I can see the helpfulness of a comment here saying: // __is_enum is a supported extension across all of our supported compilers. On the other hand, I can see how people might say this is redundant, given that the code clearly compiles. :) Up to you.

Comment on attachment 778944 [details] [diff] [review] Part 2: Make mozilla::Atomic<T> use mozilla::EnableIf for specializations Review of attachment 778944 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Atomics.h @@ +885,5 @@ > { > + private: > + Atomic() MOZ_DELETE; > + Atomic(Atomic<T, Order>& aOther) MOZ_DELETE; > +}; It would be better to simply not declare a class here at all; compiler error messages should be slightly more clear then. @@ +908,5 @@ > > + T operator++(int) { return Base::Intrinsics::inc(Base::mValue); } > + T operator--(int) { return Base::Intrinsics::dec(Base::mValue); } > + T operator++() { return Base::Intrinsics::inc(Base::mValue) + 1; } > + T operator--() { return Base::Intrinsics::dec(Base::mValue) - 1; } These should reside in a new detail::AtomicBaseIncDec or similar. Atomic<integer-type> and Atomic<T*> can then inherit from detail::AtomicBaseIncDec (and ideally not be too uglified). @@ +926,5 @@ > Atomic(Atomic<T, Order>& aOther) MOZ_DELETE; > }; > > /** > + * Atomic<T> implementation for pointer types. Why are you changing this to use EnableIf rather than just letting the template specialization mechanism work?

Comment on attachment 778945 [details] [diff] [review] Part 3: Add enum support to mozilla::Atomic<T> Review of attachment 778945 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me, though |T operator=(T a)| should probably be moved into AtomicBase as a followup. I'm not sure what's wrong with GCC 4.7; the behavior there looks like a bug. Unfortunately, that bug is going to prevent us from landing this patch. :( I see two ways forward: 1) Don't permit compareExchange on Atomic<enum>. Move it into a common base shared by Atomic<integral> and Atomic<pointer> instead. The suggested AtomicBaseIncDec from part 2 would be a great place for it, albeit with an XXX comment that indicates we know about the wrongness of putting it there, but there's nothing we can do currently. 2) File upstream bug report, get it fixed in the next 4.7.x, and upgrade to that on the builders (or backport the fix). These are not mutually exclusive. I will try reducing the testcase to file the bug. f+'ing instead of r+'ing so this patch doesn't get landed accidentally. Will r+ once we figure out a satisfactory way forward.

Addresses comment #6.

This addresses comment #7 and does not permit compareExchange on Atomic<enum T> for now. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ae599871fdf6 By the way, it may be possible to change `T operator=(T aValue) { return Base::operator=(aValue); }` to simply `using Base::operator=;`, but I don't know if all our compilers support it. I can test on try if you want.

Comment on attachment 779845 [details] [diff] [review] Part 2: Refactor and cleanup mozilla::Atomic<T> implementation Review of attachment 779845 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks for the changes!

Comment on attachment 779850 [details] [diff] [review] Add enum support to mozilla::Atomic<T> Review of attachment 779850 [details] [diff] [review]: ----------------------------------------------------------------- r=me with or without the |using Base::operator=| bit (though I'd think the normal rules of function lookup would find the operator in the base class?).

Comment on attachment 779850 [details] [diff] [review] Add enum support to mozilla::Atomic<T> Review of attachment 779850 [details] [diff] [review]: ----------------------------------------------------------------- We may run into issues here if this gets used with enums that the compiler doesn't back by a size-4 (or on 64-bit, size-8) integer type. I can't quite remember the C++ rules well enough to know what it'd take for that to happen. If you don't declare a copy assignment operator explicitly, then one is declared implicitly by the compiler. In this case the implicitly declared version would have the default implementation. A |using Base::operator=;| or explicitly declaring one gets around that. C++ is fun. See C++11 12.8 p17/18, i.e. [class.copy]p17-18. ::: mfbt/Atomics.h @@ +985,5 @@ > + typedef typename detail::AtomicBase<T, Order> Base; > + > + public: > + Atomic() : detail::AtomicBase<T, Order>() {} > + Atomic(T aInit) : detail::AtomicBase<T, Order>(aInit) {} If you're going to typedef Base, you might as well use it for both of these, seems to me.

Hmm, guess the Base bit's in the existing classes as well. Someone should driveby-cleanup that sometime, or something.

Changed to use |using Base::operator=| since it compiled fine on try. This also addresses Waldo's suggestion in comment #13.

Comment on attachment 780390 [details] [diff] [review] Part 4: Cleanup Atomic<T> Review of attachment 780390 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the cleanup!

(In reply to Birunthan Mohanathas [:poiru] from comment #4) > Created attachment 778945 [details] [diff] [review] > Part 3: Add enum support to mozilla::Atomic<T> > > For some odd reason, this patch causes the following error with GCC. Clang > and MSVC work fine. > > In file included from ../../dist/include/mozilla/Atomics.h:176:0, > from ../../../mfbt/tests/TestAtomics.cpp:6: > /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../ > include/c++/4.7.2/atomic: In function 'int main()': > /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../ > include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for > '__atomic_compare_exchange' > /tools/gcc-4.7.2-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.2/../../../../ > include/c++/4.7.2/atomic:259:69: error: invalid failure memory model for > '__atomic_compare_exchange' > > See: https://tbpl.mozilla.org/?tree=Try&rev=ba498905e72f > > > GCC is able to compile TestAtomics.cpp with the following changes. I do not > know if there exists a general solution for GCC (other than not defining > MOZ_HAVE_CXX11_ATOMICS for it). Any ideas? This is a bug in GCC's <atomic> header. Once bug 898491 goes in, could you please amend your patches to provide compareExchange for atomics and uncomment the relevant testcases?

(In reply to Nathan Froyd (:froydnj) from comment #17) > This is a bug in GCC's <atomic> header. Once bug 898491 goes in, could you > please amend your patches to provide compareExchange for atomics and > uncomment the relevant testcases? Done.

Comment on attachment 783661 [details] [diff] [review] Part 3: Add enum support to mozilla::Atomic<T> Review of attachment 783661 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: mfbt/Atomics.h @@ +989,5 @@ > + * The atomic store and load operations and the atomic swap method is provided. > + */ > +template<typename T, MemoryOrdering Order> > +class Atomic<T, Order, typename EnableIf<IsEnum<T>::value>::Type> > + : public detail::AtomicBaseIncDec<T, Order> This should be inheriting from AtomicBase; we don't want to support increment and decrement on enums. @@ +991,5 @@ > +template<typename T, MemoryOrdering Order> > +class Atomic<T, Order, typename EnableIf<IsEnum<T>::value>::Type> > + : public detail::AtomicBaseIncDec<T, Order> > +{ > + typedef typename detail::AtomicBaseIncDec<T, Order> Base; AtomicBase here as well.

(In reply to Nathan Froyd (:froydnj) from comment #20) > This should be inheriting from AtomicBase; we don't want to support > increment and decrement on enums. Heh, I guess I shouldn't write code when sleep deprived. Fixed now, thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e31542e117c https://hg.mozilla.org/integration/mozilla-inbound/rev/f607ac59de19 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc98067f0aa4

This broke B2G builds, with errors like: { 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: template argument for 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: trying to instantiate 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: template argument for 'template<class T> class mozilla::DebugOnly' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: trying to instantiate 'template<class T> class mozilla::DebugOnly' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: template argument for 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:142: error: trying to instantiate 'template<class T, mozilla::MemoryOrdering Order, class Enable> struct mozilla::Atomic' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: template argument for 'template<class T> class mozilla::DebugOnly' uses local type 'TestEnumWithOrdering() [with mozilla::MemoryOrdering Order = (mozilla::MemoryOrdering)2u]::EnumType' 18:50:51 ERROR - ../../../gecko/mfbt/tests/TestAtomics.cpp:146: error: trying to instantiate 'template<class T> class mozilla::DebugOnly' } https://tbpl.mozilla.org/php/getParsedLog.php?id=25999956&tree=Mozilla-Inbound For reference, here's the inbound cycle where this landed (with the busted builds): https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=bb8539a50e37 I backed it out: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff607d314da1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b045a3eb7507 https://hg.mozilla.org/integration/mozilla-inbound/rev/19c4b977342b The

(sorry, disregard the trailing "The" in prev. comment)

Comment 23 sounds like we're not compiling as C++11 there. In C++98 templates couldn't be instantiated parametrized by types without linkage -- including non-namespace-level defined types. In C++11 you can use local types defined inside function bodies. I'd thought this not-compiling-as-C++11 thing was bug 895915, but this landed after that, so I dunno. The easy workaround is just to move the various enums to top level, and we should probably do that in the short run. In the longer run we should figure out what's up here and fix it. (Could just be the b2g compiler's too old to support template types without linkage, of course, in which case we should be aware of the issue for next time.)

The log is clear: the failing command has -std=gnu++0x on the command line. And it's not a host build, it's a target build, so that would be bug 894242 instead of bug 895915. Anyways, it's likely one of the many weaknesses of gcc 4.4 in the C++0x department. I'd be happier if b2g could just switch to gcc 4.7 already...

(In reply to Daniel Holbert [:dholbert] from comment #23) > This broke B2G builds, with errors like: Updated patch to fix this.

> I'd be happier if b2g could just switch to gcc 4.7 already... We're going to be stuck with what we have until we're done with 1.1...

(In reply to Justin Lebar [:jlebar] from comment #28) > We're going to be stuck with what we have until we're done with 1.1... Why is 1.1 affected by what compiler we use on trunk/1.2?

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbce2a590a2b https://hg.mozilla.org/integration/mozilla-inbound/rev/99511f741225 https://hg.mozilla.org/integration/mozilla-inbound/rev/424e1cb4850c

https://hg.mozilla.org/mozilla-central/rev/dbce2a590a2b https://hg.mozilla.org/mozilla-central/rev/99511f741225 https://hg.mozilla.org/mozilla-central/rev/424e1cb4850c