Closed
Bug 965837
Opened 10 years ago
Closed 10 years ago
mozilla::Atomic should support bool
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: vikstrous, Assigned: vikstrous)
References
Details
Attachments
(1 file, 9 obsolete files)
Our current implementation of atomics doesn't support bool because it treats it as an integral type and then complains that it's not 32 or 64 bits. We should have atomic bools.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8367990 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → vstanchev
Comment 2•10 years ago
|
||
Comment on attachment 8367990 [details] [diff] [review] atomic_bool Review of attachment 8367990 [details] [diff] [review]: ----------------------------------------------------------------- This is headed in the right direction, but requires some polish. ::: mfbt/Atomics.h @@ +1008,5 @@ > > +/** > + * Atomic<T> implementation for boolean types. > + * > + * The atomic store and load operations and the atomic swap method is provided. You should have a brief blurb here about how sizeof(Atomic<bool>) != sizeof(bool) for some implementations of bool and/or some implementations of std::atomic, how this is driven by the constraints we have for atomic operations on windows, and how this behavior is explicitly allowed by the standard ([atomic.types.generic]p9). I can help with the finer points of the wording if you write the first draft. @@ +1020,5 @@ > + public: > + MOZ_CONSTEXPR Atomic() : Base() {} > + MOZ_CONSTEXPR Atomic(bool aInit) : Base(aInit) {} > + > + using Base::operator=; This is not quite right (nor are the other operations exposed by AtomicBase quite right) because those operations are on uint32_t. So we have an Atomic<bool> that allows assigning, say, UINT32_MAX to it, and getting that value back out again. While promotion/conversion rules probably make this not too awful, we should strive to keep the interface well-typed. I think you have two options: 1) Manually re-implement most of AtomicBase here, with proper bool-taking/returning types; or 2) protected inheritance from AtomicBase<uint32_t, Order> and writing bool-taking/returning wrappers around the operations. ::: mfbt/tests/TestAtomics.cpp @@ +181,5 @@ > + // Test exchange. > + atomic = false; > + result = atomic.exchange(true); > + MOZ_ASSERT(atomic == true, "Atomic exchange did not work"); > + MOZ_ASSERT(result == false, "Atomic exchange returned the wrong value"); You should test compareExchange here too, similarly to TestEnumWithOrdering.
Attachment #8367990 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8367990 -
Attachment is obsolete: true
Attachment #8368077 -
Flags: review?(nfroyd)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8368077 -
Attachment is obsolete: true
Attachment #8368077 -
Flags: review?(nfroyd)
Attachment #8368085 -
Flags: review?(nfroyd)
Comment 5•10 years ago
|
||
Comment on attachment 8368085 [details] [diff] [review] atomic_bool2 Review of attachment 8368085 [details] [diff] [review]: ----------------------------------------------------------------- Almost there, just a few more things to fix. ::: mfbt/Atomics.h @@ +1018,5 @@ > + * [atomic.types.generic]p9. > + * > + * - It's not obvious whether the 8-bit atomic functions on Windows are always > + * inlined or not. If they are not inlined those functions may not be > + * available on Windows XP. This is why we implement the atomic bool "If they are not inlined, the corresponding functions in the runtime library are not available on Windows XP." @@ +1019,5 @@ > + * > + * - It's not obvious whether the 8-bit atomic functions on Windows are always > + * inlined or not. If they are not inlined those functions may not be > + * available on Windows XP. This is why we implement the atomic bool > + * using unit32_t. "This is why we implement Atomic<bool> with an underlying type of uint32_t." @@ +1023,5 @@ > + * using unit32_t. > + */ > +template<MemoryOrdering Order> > +class Atomic<bool, Order> > + : public detail::AtomicBase<uint32_t, Order> This still exposes the uint32_t versions of the functions. You should use protected inheritance here. @@ +1031,5 @@ > + public: > + MOZ_CONSTEXPR Atomic() : Base() {} > + MOZ_CONSTEXPR Atomic(bool aInit) : Base(aInit) {} > + > + // We provide a type safe wrapper for the underlying AtomicBase "We provide boolean wrappers for the underlying AtomicBase methods."
Attachment #8368085 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8368085 -
Attachment is obsolete: true
Attachment #8368137 -
Flags: review?(nfroyd)
Comment 7•10 years ago
|
||
Comment on attachment 8368137 [details] [diff] [review] atomic_bool3 Review of attachment 8368137 [details] [diff] [review]: ----------------------------------------------------------------- I assume you are folding this into the previous patch. r=me on the whole thing if so.
Attachment #8368137 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8368137 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8368148 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f434624463d9
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•10 years ago
|
||
Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/09e27ba697c2 https://tbpl.mozilla.org/php/getParsedLog.php?id=33833357&tree=Mozilla-Inbound
Comment 12•10 years ago
|
||
So, the fun of C++. [class.conv]p5 says that conversion functions from a derived class do not hide conversion functions in a base class unless they both convert to the same type. That makes some amount of sense. [class.conv]p2 says that access control is applied *after* ambiguity resolution. So |operator uint32_t()| and |operator bool()| are both available in an expression like |atomicBool == false|, even though |operator uint32_t()| isn't actually accessible. So why are we selecting |operator uint32_t()| in the first place? I believe this is because, according to the table at [over.ics.scs]p3, |operator uint32_t()| counts as an integral promotion, whereas |operator bool()| counts as a boolean conversion. Promotion is to be preferred over conversion for the purposes of overload resolution, so |operator uint32_t()| gets selected. There's also some verbiage at [over.ics.user] that probably applies to this case, but I think that is the jist of it. (Implicit promotion to int may also take effect here, which is another wrinkle.) Viktor just pointed out http://en.cppreference.com/w/cpp/language/implicit_cast and http://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool, too. C++ is awful.
Comment 13•10 years ago
|
||
This problem keeps coming up. Somebody should just add the infrastructure for the safe bool idiom to MFBT so that we can start using it.
Assignee | ||
Comment 14•10 years ago
|
||
The problem in this particular case is not really with the safe bool itself. The safe bool would be nice improvement, but the real issue is that we have two () operators and they both match when comparing with a bool. There are two possible ways for == to compare them, so it complains that the statement is ambiguous. I don't think we really need safe bools for this to work. They just provide additional compile time checks so that no one can write a == b where a is Atomic<bool> and b is unit_32.
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8368157 -
Attachment is obsolete: true
Attachment #8368696 -
Flags: review?(nfroyd)
Assignee | ||
Comment 16•10 years ago
|
||
(typo)
Attachment #8368696 -
Attachment is obsolete: true
Attachment #8368696 -
Flags: review?(nfroyd)
Attachment #8368699 -
Flags: review?(nfroyd)
Comment 17•10 years ago
|
||
(In reply to comment #14) > The problem in this particular case is not really with the safe bool itself. > The safe bool would be nice improvement, but the real issue is that we have two > () operators and they both match when comparing with a bool. There are two > possible ways for == to compare them, so it complains that the statement is > ambiguous. I don't think we really need safe bools for this to work. They just > provide additional compile time checks so that no one can write a == b where a > is Atomic<bool> and b is unit_32. Oh, sorry, yes. As long as you have a uint32_t conversion, you're doomed. :-)
Comment 18•10 years ago
|
||
Comment on attachment 8368699 [details] [diff] [review] 8368696: Bug 965837 - Add bool support to mozilla::Atomic Review of attachment 8368699 [details] [diff] [review]: ----------------------------------------------------------------- I only super-skimmed the logic/test bits, so don't take this as a review. :-) I'll never remember the argument meanings for compareAndSwap and exchange and the non-trivial cases without looking it up each time. (Or doing lots, lots more atomic code than I do at present.) ::: mfbt/Atomics.h @@ +845,5 @@ > MOZ_CONSTEXPR AtomicBase() : mValue() {} > MOZ_CONSTEXPR AtomicBase(T aInit) : mValue(aInit) {} > > + // Note: we can't provide operator T() here because Atomic<bool> inherits > + // from AtimcBase with T=unit32_t and not T=bool. If we implemented AtomicBase
Comment 19•10 years ago
|
||
Comment on attachment 8368699 [details] [diff] [review] 8368696: Bug 965837 - Add bool support to mozilla::Atomic Review of attachment 8368699 [details] [diff] [review]: ----------------------------------------------------------------- r=me with Jeff's comments addressed too. Please make sure to build debug on at least one major platform before pushing. :) ::: mfbt/Atomics.h @@ +845,5 @@ > MOZ_CONSTEXPR AtomicBase() : mValue() {} > MOZ_CONSTEXPR AtomicBase(T aInit) : mValue(aInit) {} > > + // Note: we can't provide operator T() here because Atomic<bool> inherits > + // from AtimcBase with T=unit32_t and not T=bool. If we implemented T=uint32_t @@ +847,5 @@ > > + // Note: we can't provide operator T() here because Atomic<bool> inherits > + // from AtimcBase with T=unit32_t and not T=bool. If we implemented > + // operator T() here, it would cause errors when comparing Atomic<bool> with > + // a regular bool Period at the end of sentence.
Attachment #8368699 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8368699 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8369563 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c85be9b269a3
Assignee | ||
Comment 23•10 years ago
|
||
Oops... the previous push to try didn't have osx and wasn't a debug build. Here's another one: https://tbpl.mozilla.org/?tree=Try&rev=c0767287d956
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb87577b5221
Keywords: checkin-needed
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fb87577b5221
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•