On 2014-04-01, 6:15 PM, Rob Arnold wrote:
I've got some outside experience with std::atomic so make of it what you
will :)
How often are you touching atomic variables directly? In my experience
with a similar thread safe inline ref count and smart pointer system to
Mozilla's (using std::atomic for the ref count), there's been no
confusion as the equivalent ref counted base class is super small and
easy to read/verify.
I was hesitant to give the refcount example to avoid confusing people to
think I'm just talking about refcounts, but I'm glad you got the point!
In the case of that mRefCnt variable, I originally did not pay attention
to the bug in the code because nothing about |mRefCnt| looked like it's
an atomic variable. To be honest, I don't go and check the type of
every single variable that I manipulate in my patches, in a lot of cases
I rely on cues in the surrounding code to tell me something about the
type (unconsciously thinking, this thing is called mRefCnt, and it has
operator++ and --, so it's got to be an integer.)
I cannot speak why the reviewers did not catch this bug, but I suspect
because of similar reasons.
> In the other cases where std::atomic occurs,
careful consideration is already given to threading due to the nature of
the code so the similarity to a non-atomic integer type actually has
prompted me to have a (temporary) false sense of thread unsafety, rather
than a false sense of thread safety. And I think that is fine. For my
team's code, I think it's fine that they have similar notation as it
makes operations (like statistics counters) fairly easy to read vs
explicit atomicIncrement(&counter, value) calls.
I agree that in code which does obvious threading, my proposal won't
improve things, but I'm mostly worried about bugs creeping in where we
use atomics for things that are not obviously threading related.
Cheers,
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
