2

I am trying to write a move assignment operator for a class that contains an atomic variable. Since atomic's are not movable as per this answer, I've realized that I have to write a move assignment operator that loads the atomic and then stores it. But, I then have to manually call move on all the other fields in my class. Is there a shorter way to do this (assuming all the other fields are movable)?

class Test {
    std::atomic<int*> val{nullptr};
    // MANY MORE FIELDS HERE

    Test& operator=(Test&& other) {
        this->val = other.val;
        // init the many other fields here
        return *this;
    }
};
  • 2
    Consider writing a small helper class that contains only the atomic member as public and which implements move semantics correctly. Then you could use that helper type instead of the atomic and the compiler generated operators and constructors would be correct. – François Andrieux Jan 14 '21 at 19:38
  • 1
    Also consider whether you need this to be an expensive seq_cst store. Or for the load to be `seq_cst`, which costs extra on many non-x86 platforms vs. weaker loads. If you're moving from the old object, presumably it's no longer valid so it would be UB for any other threads to still possibly be accessing its atomic member. So you might want `other.val.load(std::memory_order_relaxed)`, or possibly acquire, although anything like `.join` or a syncs-with should transitively guarantee visibility of stuff older than the member values. (Avoiding seq_cst on the store helps much more on most CPUs.) – Peter Cordes Jan 14 '21 at 19:42
  • What are your expectations for that assignment operators? What are the exact atomicity issues? What could other threads do? – curiousguy Jan 17 '21 at 02:54
  • This move in particular doesn't have to be atomic, no other thread is accessing this data yet (this move was in a conditional that may or may not be happen before exposing the class to other threads). – Anubhav Srivastava Jan 18 '21 at 01:42

2 Answers2

3

Consider

class Test {
    std::atomic<int*> val{nullptr};
    struct Movables
    {
        // MANY MORE FIELDS HERE
    } movables;

public:
    Test& operator=(Test&& other) {
        this->val.exchange(other.val);
        this->movables = std::move(other.movables);
        return *this;
    }
};
Drew Dormann
  • 59,987
  • 13
  • 123
  • 180
  • 1
    I'm going to leave my answer up, but I like this better as it keeps the internals encapsulated in the class. +1. – NathanOliver Jan 14 '21 at 19:54
  • @NathanOliver haha, I was just considering deleting my answer, as it's not an accident that atomics are not movable. Both our answers come at the cost of making the atomic _**not really atomic anymore**_. Thoughts? – Drew Dormann Jan 14 '21 at 19:57
  • 1
    That is something to consider. I guess it really depends on what `val` is being used for. Since non of the other members are atomic, I feel its safe to say the OP knows their assignment operator wont be an atomic operation. – NathanOliver Jan 14 '21 at 20:01
  • Oh nice I like the encapsulation in this! I'm not sure what you mean by "the atomic isn't really atomic anymore", though. Practically this doesn't matter in my case, I'm assigning an object that's never exposed to more than one thread to one that hasn't been exposed yet but will be, but I'm still curious. – Anubhav Srivastava Jan 14 '21 at 20:30
  • 1
    @AnubhavSrivastava - I mean specifically that reading from one atomic and storing the result in a different atomic _is not an atomic operation_. It's two operations. That might be fine for your purposes... it just doesn't satisfy the textbook definition of "atomic". – Drew Dormann Jan 14 '21 at 21:22
3

One shorter way I could see to do this would be to move MANY MORE FIELDS HERE into a different class and make that a base class of Test, Then you just have one line of code to call the move assignment operator of the base class. That would look like

class Base {
public:
    // MANY MORE FIELDS HERE
};

class Test : private Base {
    std::atomic<int*> val{nullptr};
    
public:
    Test& operator=(Test&& other) {
        this->val = other.val;
        Base::operator=(std::move(other));
        return *this;
    }
};
NathanOliver
  • 171,901
  • 28
  • 288
  • 402