On 4/1/2014 4:32 PM, Ehsan Akhgari wrote:
So, over in bug 987887 I'm proposing to remove all of the methods on mozilla::Atomic except for copy construction and assignment and replace them with global functions that can operate on the atomic type. <atomic> has a number of global functions that can operate on std::atomic types < http://en.cppreference.com/w/cpp/atomic> and we can look into implementing similar ones (for example, mozilla::AtomicFetchAdd, mozilla::AtomicExchange, etc. -- names to be bikeshedded outside of this thread!)
I am very much against this, and I think your proposal is a "medicine" which is far more harmful than the "problem" ever is or was.
My original design for mozilla::Atomic was meant to avoid what I saw as the biggest footgun: you cannot misuse mozilla::Atomic in such a way that you combine atomic and non-atomic accesses on a single variable. You also cannot mix-and-match different memory orders on the same atomic variable (in this manner, I willfully departed from std::atomic). Using global functions instead would tend to cause people to want to draw this information from the point of declaration to the point of use: note that it is much easier in tooling to find the declaration of a variable than it is to find all uses of one.
There are other issues with your design. The verbose names have, to me at least, been a constant source of confusion: I can never recall if FetchAndAdd returns the value prior to or subsequent to the addition; note that operator+= does not have the same problems.
As for the original problem, I think you're misdiagnosing the failure. The only real problem of the code is that it's possible that people could mistake the variable for not being an atomic variable. Taking a look through most of the uses of atomics in our codebase, the cases where people are liable to not realize that these are atomics are relatively few, especially if patch authors and reviewers are both well-acquainted with the surrounding code. So it's not clear to me that including extra-verbose names would really provide much of a benefit. Instead, I think you're overreacting to what is a comparatively rare case: a method templated to support both non-thread-safe and thread-safe variants, and much less frequently used or tested in thread-safe conditions (note that if you made the same mistake with nsISupports-based refcounting, your patch would be fail sufficiently many tests to be backed out of mozilla-inbound immediately).
-- Joshua Cranmer Thunderbird and DXR developer Source code archæologist _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
