On 2014-04-02, 12:11 AM, Joshua Cranmer 🐧 wrote:
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.
What roc said. Really nothing in my proposal would invalidate what you
said above. To make things super clear, I am only proposing a syntax
change.
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.
Please suggest better names then, I'm pretty open to any other name that
you think is clearer. You can't invalidate my proposal by saying you
don't like the names. :-)
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.
Yes, that is exactly what the issue is!
> Taking a
look through most of the uses of atomics in our codebase,
FWIW I'm much more interested in code that we'll write in the years to
come. I don't look forward to people running into this footgun over and
over again from now on.
> 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).
See dbaron's reply here:
<https://groups.google.com/d/msg/mozilla.dev.platform/nMfQAHeAmiA/GNXkqrKzLKQJ>.
I never claimed that this will help reviewers catch 100% of the
problems, but I don't see how you can claim that it doesn't help at all!
Cheers,
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
