C++ move constructor for an owning pointer member

Question

I'm asking something that looks trivial but I had problems with. Let's assume, for the sake of explanation, a structure like this one:

class MyClass{
    int* m_Number;
public:
    int value() const {return *m_Number;}
    void setValue(int val){*m_Number=val;}
    MyClass() : m_Number(new int(3)){}
    ~MyClass() {if(m_Number) delete m_Number;}
    MyClass(const MyClass& other):m_Number(new int(*other.m_Number)){}
    MyClass& operator=(const MyClass& other){if(m_Number) *m_Number=*other.m_Number; else m_Number=new int(*other.m_Number); return *this;}
    MyClass& operator=(MyClass&& other){std::swap(m_Number,other.m_Number); return *this;}
    MyClass(MyClass&& other){
            //????
    }
}

What should I put in there? My options are:

1)

    MyClass(MyClass&& other)
            :m_Number(other.m_Number)
    {
            other.m_Number=nullptr;
    }

But then the moved from object is not in a valid state. calling value() should return something valid but undetermined while here I just segfault. I could check m_Number in value() and setValue() but you realise it's a huge drag in real code.

---

2)

    MyClass(MyClass&& other)
            :m_Number(other.m_Number)
    {
            other.m_Number= new int(3);
    }

But a move constructor that can throw is a no go (or at least as I understand it) and it's also a drag of the performance enhancement, in fact this code is equal or worse that the copy constructor.

What do you think?

Did I miss something?

Is there a preferred way to go?

Thanks in advance

Edit: This question received an answer from the convener of the std committee and it fundamentally disagrees with the answers to this post. You can find it in this article https://herbsutter.com/2020/02/17/move-simply/

Answer 1

Firstly, there's no reason to use new and delete here, you should be using make_unique<int> to create the object and a unique_ptr<int> to manage it automatically. But that doesn't solve your problem, of what the move constructor can do. There are some other options in addition to the two you suggest:

3) don't give it a move constructor, just leave it as copyable

4) document that calling value or setValue on a moved-from object is not allowed, and leave the moved-from object with a null pointer. Depending where moves happen in your program that might be fine. If moved-from objects are never accessed, everything just works.

4a) As above, but add sanity checks in case it does ever happen:

int value() const {
  assert(m_Number != nullptr);
  return *m_Number;
}

or:

int value() const {
  if (m_Number == nullptr)
    throw std::logic_error("accessed a moved-from object");
  return *m_Number;
}

5) Add checks to setValue to re-initialize the object with a new int if it's currently null, and make value return some default value:

int value() const { return m_Number ? *m_Number : 0; }

void setValue(int val) {
  if (!m_Number)
    m_Number = new int(val);
  else
    *m_Number = val;
}

Answer 2

You are dereferencing your pointer in the .value() call. You will always segfault in the event that m_Number is invalid.

Your first solution to the move constructor is correct, you should set the 'other' object to a default state. To solve this, you could make your .value() method throw, or return a default value in the event of non-existent resource.

Your destructor already accounts for the null case, so make sure the rest of it accounts for it as well.

Answer 3

1. Your first approach is probably the best if modifying MyClass such that m_Number = nullptr is a valid state is reasonable to do (and would be best practice if it were the default state too). I'd argue if having no-heap memory associated with MyClass is not a valid state, you should be allocating it inside of a std::unique_ptr, and passing around a raw pointer to that instead of implementing a move constructor.

2. If 1. is not an option, this is a reasonable way to go. While there certainly is a performance hit for allocating un-needed memory, it is quite minor, especially considering you are constructing a class as part of this operation (which itself requires memory). If its a small chunk of memory that is needed as in your example, the memory allocator will likely draw it from its own pool without the need of a system call. If it is a large chunk of memory (as I would expect since you're implementing move semantics), then on most modern operating systems it will be a lazy allocation (so it would still be better than a copy constructor).

Answer 4

Do not use raw owning pointers, convert your class to use std::unique_ptr, and all your problems will go away.