pimpl move semantic error management

VRonin

This topic references style conventions described here

Hi everyone.
Let's talk move semantic and pimpl idiom. They seem to be natural partners and they are. the only problem I'm facing regards operations on moved objects.
My move constructors pretty much follow this scheme:
MyClass(MyClass&& other) : d_ptr(other.d_ptr) {other.d_ptr=nullptr; d_ptr->q_ptr = this;}

I set the moved object d-pointer to null because a moved object is still assignable so I can do:
MyClass& operator=(const MyClass& other){if(!d_ptr) d_ptr = new MyClassPrivate(this); /*rest of the assignament*/}

now my problem is that in my code there is a chance of d_ptr being null. Would you assert it every time you use it like:
#define SAFEQ_D(x) Q_ASSERT_X(d_ptr,"SAFEQ_D","Trying to act on moved object"); Q_D(x) and force yourself to use SAFEQ_D every time you access the d-pointer (with obvious problems when a method of MyClass accepts a MyClass argument) or do you just let it sigsev and hope the programmer will figure out it's because of the move?

In other words, if you are using MyClass from a 3rd party library and you do this by mistake:

MyClass a;
MyClass b(std::move(a));
qDebug() << a.value(); // BOOM!

would you want the library to warn you the error is in accessing a moved object or would you be fine with a sigsev due to accessing a null d_ptr and leaving up to you to figure out what went wrong?

jsulm

@VRonin Interesting question.
You could assign a new private instance to d_ptr instead of assigning a nullptr.
Not an efficient solution but still better than checking d_ptr all the time.

VRonin

The problem with that is that the advantages of the move constructor disappear as instead of calling a copy constructor on the new object you call a default constructor on the old one. In the vast majority of cases (i.e. constructors/assignments from functions returning the same type) this overhead is totally unnecessary.
Lastly that would prevent me to declare the move constructor as noexcept

Konstantin Tokarev

You should swap d-pointers in your move operations

Paul Thompson

Depending on the requirements of the library client, I'd be tempted to leave it up to the client programmer to make sure the objects are managed properly. If they try to do something with an object which has had its guts moved, it's their problem, because presumably, they've moved it!

That said, I've had to water down that attitude in the past when writing library code, just to get some forward motion.

VRonin

@Konstantin-Tokarev said in pimpl move semantic error management:

You should swap d-pointers in your move operations

In the move constructor I just have 1 valid d-pointer so even if I swap instead of segfault on nullptr it will segfault on a dangling pointer which is even worse

VRonin

@Paul-Thompson said in pimpl move semantic error management:

If they try to do something with an object which has had its guts moved, it's their problem, because presumably, they've moved it!

I agree but I also imagine being on the other side and having to debug this might turn out to be a nightmare if I don't tell them the problem is caused by a move somewhere

VRonin

I'm checking what Qt did: https://codereview.qt-project.org/#/c/159085/5/src/corelib/tools/qdatetime.cpp

Basically they detach if you try to use the moved object and lets you use it as if you didn't move it in the first place but rather just used the copy constructor. I guess for implicitly/explicitly shared data it's fine as even the copy constructor is cheap (actually, looking at the code above there's hardly any benefit coming from using move instead of copy constructor at least on 32bit platform).

Thoughts?

kshegunov

@VRonin said in pimpl move semantic error management:

My move constructors pretty much follow this scheme

Which is the default move (and copy) constructor for any integral type, which pointers are.

would you want the library to warn you the error is in accessing a moved object or would you be fine with a sigsev due to accessing a null d_ptr and leaving up to you to figure out what went wrong?

I'd just leave it as is - i.e. use the default. No library, or a language for that matter, is smart enough to prevent you from shooting yourself in the foot if you don't care to follow the rules. And C++ even can even facilitate it. In any case if someone wants to blow their leg off, why would you stop them from having fun?

Basically they detach if you try to use the moved object and lets you use it as if you didn't move it in the first place but rather just used the copy constructor.

Well, that's what "moving" a pointer is really in the sense of implicit sharing, and it's equivalent to copying.

actually, looking at the code above there's hardly any benefit coming from using move instead of copy constructor at least on 32bit platform

That scheme was invented and perfected probably a decade or two before the committee even conceived move semantics, it's just another way to solve the problem, with the additional advantage that it does not require a modification to the language.

VRonin

@kshegunov said in pimpl move semantic error management:

That scheme was invented and perfected probably a decade or two before the committee even conceived move semantics, it's just another way to solve the problem, with the additional advantage that it does not require a modification to the language

Yes, what I really meant is that I can't just wwqd (what would Qt do?) my way out of my problem but I have to think something myself.

I'm also not 100% convinced by the not-my-problem approach as that means I'm ruining somebody's day somewhere sometime and I can't be ok with it.

I'm also not that convinced if asserting should be the way to go or you should allow, as Qt does, to use moved from object as totally valid objects.

Atm my favourite option is to replace Q_DECLARE_PRIVATE with:

#define S_DECLARE_PRIVATE(Class) \
    inline Class##Private* d_func() { Q_ASSERT_X(d_ptr,"d_func","Trying to act on moved object"); return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \
    inline const Class##Private* d_func() const {  Q_ASSERT_X(d_ptr,"d_func","Trying to act on moved object"); return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr)); } \
    friend class Class##Private;

or (but I'd need to find a way to make it work for both raw and smart pointers)

#define S_DECLARE_PRIVATE(Class) \
    inline Class##Private* d_func() { if(!d_ptr) d_ptr=new Class##Private(this); return reinterpret_cast<Class##Private *>(qGetPtrHelper(d_ptr)); } \
    inline const Class##Private* d_func() const {  if(!d_ptr) d_ptr=new Class##Private(this); return reinterpret_cast<const Class##Private *>(qGetPtrHelper(d_ptr)); } \
    friend class Class##Private;

as that allows me:

To handle the nullptr case manually when I want by checking d_ptr instead of d_func
To minimise manual refactoring of pre-C++11 code
In methods with signatures like MyClass::foo(const MyClass& other), to implicitly check both this->d_ptr and other.d_ptr just by accessing their d_func (which is what I do already)

Happy to hear opinions and drawbacks I did not realise

kshegunov

@VRonin said in pimpl move semantic error management:

Yes, what I really meant is that I can't just wwqd (what would Qt do?) my way out of my problem but I have to think something myself.

Then adhere to the standard (library's) behavior strictly (as Qt does, by the way).

17.6.5.15 Moved-from state of library types
Objects of types defined in the C++ standard library may be moved from (12.8). Move operations may be explicitly specified or implicitly generated. Unless otherwise specified, such moved-from objects shall be placed in a valid but unspecified state.

I'd just do:

MyClass(MyClass && other)
    : d_ptr(std::move(other.d_ptr))
{
}

and leave the implicit sharing to take care of it (i.e. detach on demand). No modifications to the macros, or behavior of the implicit sharing is needed in this case. And there's also no difference for non-copyable objects (e.g. QObject).

*EDIT: *
If your data is big-ish, then just create an empty (default initialized) private object in the copy constructor. This should solve your conundrum (again, no modifications to the macros).

but I'd need to find a way to make it work for both raw and smart pointers

Elaborate on exactly what types of smart pointers and on the type of classes you want this to work for - do they allow copying, if so how should this be implemented, e.g. by deep copy or virtual copy constructor or implicit/explicit sharing.

VRonin

@kshegunov said in pimpl move semantic error management:

leave the implicit sharing to take care of it

I would, except my classes do not share. calling the copy constructor makes a deep copy, no detach is in place (hence I'm trying to implement at least the move semantic to increase efficiency. If I had shared data I wouldn't even bother implementing the move as gain would be negligible)

if your data is big-ish, then just create an empty (default initialized) private object

Since 99.9% of the times this will be triggered as constructor/assignment from function return, creating a default private object is a huge cost for the safety in a marginal case (such as the one where a moved object get reused). On top of that creating a default private object implies heap allocation and that implies the move constructor might end up throwing exception and that's quite bad as I understand

Elaborate on exactly what types of smart pointers and on the type of classes you want this to work for - do they allow copying, if so how should this be implemented, e.g. by deep copy or virtual copy constructor or implicit/explicit sharing.

It's actually trivial, I made it sound worse than it is. it's basically the same as qGetPtrHelper. something like (still unsure if val needs to be a const reference or I can live with passing by value)

template <typename T> static inline T *setPtrHelper(T *ptr, T* val) { ptr = val; }
template <typename Wrapper> static inline typename Wrapper::pointer setPtrHelper(const Wrapper &p, Wrapper::pointer val) { p.reset(val); }

Then adhere to the standard (library's) behavior strictly

For what I understand the standard specifies that reusing a moved object should be valid but in an unspecified state (i.e. probably rules out my Q_ASSERT_X solution but the second one is still alive) but I might be completely wrong.

Final note:

No modifications to the macros

I do not intend to change the existing macros, I'm not looking for trouble, I'm just defining a new one with a very similar body

kshegunov

@VRonin said in pimpl move semantic error management:

I would, except my classes do not share. calling the copy constructor makes a deep copy, no detach is in place

Okay. Using explicit (or implicit sharing) is not an option here?

creating a default private object is a huge cost for the safety in a marginal case

Fine, then you're free to explicitly call this "undefined behavior", as you're the designer, and leave the d-ptr either dangling or null. Tripping an assertion is probably okay, but I wouldn't really go beyond that if performance is an issue, especially as you say it is.

On top of that creating a default private object implies heap allocation and that implies the move constructor might end up throwing exception and that's quite bad as I understand

You mean that you constructor might throw an exception, or are you referring to insufficient memory exception? In the first case, although not strictly prohibited, I'd frown upon it - I like exception-free constructors. If you mean the latter, then you are probably not going to be able to handle it anyway, so I wouldn't worry so much about it.

still unsure if val needs to be a const reference or I can live with passing by value

Probably doesn't really matter (performance-wise) either way for the reference part. As for the const - you can't make it const, as you're going to take ownership of that object, unless you're going to strip the const away with a const_cast, but that's a bad, bad idea in my opinion.

For what I understand the standard specifies that reusing a moved object should be valid but in an unspecified state (i.e. probably rules out my Q_ASSERT_X solution but the second one is still alive) but I might be completely wrong.

As far as I know this is the standard library's behavior, not part of the language standard. So you're free to design your code elsewise.

I do not intend to change the existing macros, I'm not looking for trouble, I'm just defining a new one with a very similar body

Yes, that's what I thought, I didn't mean to imply you're about to go on a rampage changing Qt's headers. ;)

VRonin

I'll close this one as I feel all options are on the table.
To summarise, you can either:

Call the move constructor of the private class that in turn will call the move constructor of each member (and maybe some more overhead)
Use shared data (in which case you probably don't really need a move constructor anyway)
Check d_ptr before accessing it in every method

VRonin

forgot to mention. remember to set the q_ptr of the moved private class to the new public one. edited code above

Eddy

Hi VRonin,

The only Qt Class I can think off that doesn't use Q_DISABLE_COPY (which disables copy constructors ...) is QFileInfo :

#ifdef Q_COMPILER_RVALUE_REFS
    QFileInfo &operator=(QFileInfo &&other) Q_DECL_NOTHROW { swap(other); return *this; }
#endif

You might get some ideas there.

hope it helps.

Eddy

VRonin

@Eddy QDateTime actually was the first one to embrace the new semantic, see link above. However since Qt uses shared memory already, move semantic is a lot easier to implement but on the other hand it does not provide much of a performance improvement

Eddy

@VRonin
obviously I missed that link and you already did your homework ;-)