Solved pimpl move semantic error management
-
@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.
-
@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 checkingd_ptr
instead ofd_func
- To minimise manual refactoring of pre-C++11 code
- In methods with signatures like
MyClass::foo(const MyClass& other)
, to implicitly check boththis->d_ptr
andother.d_ptr
just by accessing theird_func
(which is what I do already)
Happy to hear opinions and drawbacks I did not realise
- To handle the
-
@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.
-
@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
-
@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. ;)
-
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
-
forgot to mention. remember to set the q_ptr of the moved private class to the new public one. edited code above
-
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
-
@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
-
@VRonin
obviously I missed that link and you already did your homework ;-)