How to avoid move semantics for class members?

Question

Example:
I have the following code (reduced to model example, Qt library used, Qt classes behavior explained below):

struct Test_impl {
  int x;
  Test_impl() : x(0) {}
  Test_impl(int val) : x(val) {}
};

class Test {
  QSharedPointer<Test_impl> m;
public:
  Test() : m(new Test_impl()) {}
  Test(int val) : m(new Test_impl(val)) {}
  void assign(const QVariant& v) {m = v.value<Test>().m; ++m->x;}
  ~Test(){--m->x;}
};

Q_DECLARE_METATYPE(Test)

QSharedPointer is a smart pointer implementing move semantic (which is omitted in documentation). QVariant is somewhat similar to std::any and has the template method

template<typename T> inline T value() const;

Macro Q_DECLARE_METATYPE allows values of type Test to be placed inside a QVariant.

Problem:
Line m = v.value<Test>().m; seemingly invokes move assignment for the field m of a temporary object returned by value(). After that, Test destructor is called and promptly crashes trying to access illegal address.
Generally, the problem as I see it is that while move assignment leaves the object itself in a consistent state, state of an object containing moved entity "unexpectedly" changes.

There are several ways to avoid the problem in this particular example I can think of: change Test destructor to expect null m, write template template<typename T> inline T no_move(T&& tmp) {return tmp;}, explicitly create temporary Test object in assign, add getter for m and call it to force copying m (advised by Jarod42); MS Visual Studio allows to write std::swap(m, v.value<Test>().m), but this code is illegal.

Question(s):
Is there a "proper" (best practice?) way to explicitly invoke copy assignment (or somehow properly invoke swap) instead of move? Is there a way to prohibit move semantics for class fields used in the destructor? Why moving a temporary object member was made a default option in the first place?

Answer 1

class Test {
  QSharedPointer<Test_impl> m;
public:
  Test() : m(new Test_impl()) {}
  Test(int val) : m(new Test_impl(val)) {}
  Test(Test const&)=default;
  Test& operator=(Test const&)=default;
  void assign(const QVariant& v) {
    *this = v.value<Test>();
    ++m->x;
  }
  ~Test(){--m->x;}
};

Your code doesn't support move construction, as you want never empty, and moving a QSharedPointer apparently makes it empty.

By explicitly =defaulting the copy constructor (and assign) we block automatic synthesis of the move constructor (and assignment).

By not accessing the fields while they are rvalues, and instead first copying them, we avoid them clearing their own state while the surrounding class expects them to store state.

This should prevent your crash, but doesn't fix your design problem.

---

In general, using a QSharedPointer in a way that presumes it cannot be null is bad form. It is a nullable type, nullable types can be null.

We can fix this by stopping that assumption. Or we can write a non-nullable shared pointer.

template<class T>
struct never_null_shared_ptr:
  private QSharedPointer<T>
{
  using ptr=QSharedPointer<T>;
  template<class...Args>
  never_null_shared_ptr(Args&&...args):
    ptr(new T(std::forward<Args>(args)...))
  {}
  never_null_shared_ptr():
    ptr(new T())
  {}
  template<class...Ts>
  void replace(Ts&&...ts) {
    QSharedPointer<T> tmp(new T(std::forward<Ts>(ts)...));
    // paranoid:
    if (tmp) ((ptr&)*this) = std::move(tmp);
  }
  never_null_shared_ptr(never_null_shared_ptr const&)=default;
  never_null_shared_ptr& operator=(never_null_shared_ptr const&)=default;
  // not never_null_shared_ptr(never_null_shared_ptr&&)
  ~never_null_shared_ptr()=default;
  using ptr::operator*;
  using ptr::operator->;
  // etc
};

Simply using to import parts of the API of QSharedPointer you want to support and do not permit you to reset the value in the pointer.

Now the type never_null_shared_ptr enforces that invariant of it not being empty.

Note that constructin a never_null_shared_ptr from a pointer is not advised. Instead you forward construct it. If you really need that, you should have it throw if the pointer passed in is null, preventing construction from occurring. This will also possibly require fancy SFINAE.

In practice, this kind of runtime checking is worse than static checking, so I'd just remove the from-pointer constructor.

Giving us:

class Test {
  never_null_shared_ptr<Test_impl> m;
public:
  Test() : m() {}
  Test(int val) : m(val) {}
  void assign(const QVariant& v) {m = v.value<Test>().m; ++m->x;}
  ~Test(){--m->x;}
};

In essence, with the substitution of the nullable shared ptr with a non-nullable shared ptr, and replacing new calls to placement constructs, your code as written suddenly works.