PDA

View Full Version : Segfault while using Pimpl idiom with Qt types.



darkadept
12th June 2009, 17:40
I'm starting to use the Pimpl idiom (aka cheshire cat, d-pointer) but I'm getting segfaults in my destructors under certain conditions. Let me try to explain:



//MYCLASS.H

class MyClassPrivate;

class MyClass {
public:
MyClass();
MyClass(const MyClass &other);
~MyClass();

private:
MyClassPrivate *d;
}




//MYCLASS.CPP
#include "myclass.h"
#include <QString>
#include <QMap>

class MyClassPrivate {
public:
QString mystring;
QMap<QString, int> mymap;
};

MyClass::MyClass()
: d(new MyClassPrivate)
{ }

MyClass:MyClass(const MyClass &other)
: d(newMyClassPrivate(*other.d))
{ }

MyClass::~MyClass()
{
delete d;
}


I have another class, let's call it MyContainerThingy that also uses Pimpl and stores a MyClass value:



//MYCONTAINERTHINGY.H

class MyContainerThingyPrivate;

class MyContainerThingy {
public:
MyContainerThingy();
MyContainerThingy(const MyContainerThingy &other);
~MyContainerThingy();

private:
MyContainerThingyPrivate *d;
}




//MYCONTAINERTHINGY.CPP
#include "mycontainerthingy.h"
#include "myclass.h"

class MyContainerThingyPrivate {
public:
MyClass myclass;
};

MyContainerThingy::MyContainerThingy()
: d(new MyContainerThingyPrivate)
{ }

MyContainerThingy:MyContainerThingy(const MyContainerThingy &other)
: d(newMyContainerThingyPrivate(*other.d))
{ }

MyContainerThingy::~MyContainerThingy()
{
delete d;
}


These two classes are compiled into a shared library. When I attempt to use these classes from my application it segfaults with the following frame stack (from KDevelop3 debugging via gdb):



#0 QBasicAtomicInt::deref
#1 ~QMap
#2 ~MyClassPrivate
#3 ~MyClass
#4 ~MyContainerThingy
#5 ~MyContainer
#6 main


And it's not only QMap, but QHash, QString as well.

Any ideas?

lni
12th June 2009, 17:58
Of course it will crash.

MyClass contains a pointer to MyClassPrivate, but your MyContainerThingyPrivate's default ctor performs a shallow copy. You need to provide a deep copy ctor.

You can use QSharedData to take care of the ref count for you...

darkadept
12th June 2009, 18:03
Ahhh. ok makes sense in the brain, now let's see what happens in implementation. I'll go try it right away. thanks

darkadept
12th June 2009, 18:06
Umm, should I be inheriting QSharedDataPointer for MyClass and MyContainerThingy as well or just inheriting QSharedData for my private classes?


EDIT: Nevermind. I should read the documentation before I blurt things out.

lni
12th June 2009, 18:39
It appears you want to use QExplicitlySharedDataPointer, which will call detach automatically when the data object is modified. But it depends on your needs...

QVector uses QSharedDataPointer...

EDIT: Error, I mean to say QExplicitlySharedDataPointer does NOT call detach automatically

darkadept
12th June 2009, 21:12
Here is my updated code using QExplicitlySharedDataPointers:



//MYCLASS.H

class MyClassPrivate;

class MyClass {
public:
MyClass();
MyClass(const MyClass &other);
~MyClass();

private:
QExplicitlySharedDataPointer<MyClassPrivate> d;
};




//MYCLASS.CPP
#include "myclass.h"
#include <QString>
#include <QMap>

class MyClassPrivate : public QSharedData {
public:
MyClassPrivate() { }
MyClassPrivate(const MyClassPrivate &other) : QSharedData(other), mystring(other.mystring), mymap(other.mymap) { }
QString mystring;
QMap<QString, int> mymap;
};

MyClass::MyClass()
: d(new MyClassPrivate)
{ }

MyClass::MyClass(const MyClass &other)
: d(other.d)
{ }

MyClass::~MyClass()
{ }




//MYCONTAINERTHINGY.H

class MyContainerThingyPrivate;

class MyContainerThingy {
public:
MyContainerThingy(const MyClass &cls);
MyContainerThingy(const MyContainerThingy &other);
~MyContainerThingy();

private:
QExplicitlySharedDataPointer<MyContainerThingyPrivate> d;
};




//MYCONTAINERTHINGY.CPP
#include "mycontainerthingy.h"
#include "myclass.h"

class MyContainerThingyPrivate : public QSharedData {
public:
MyContainerThingyPrivate() { }
MyContainerThingyPrivate(const MyContainerThingyPrivate &other) : QSharedData(other), myclass(other.myclass) { }
MyClass myclass;
};

MyContainerThingy::MyContainerThingy(const MyClass &cls)
: d(new MyContainerThingyPrivate)
{
d->myclass = cls;
}

MyContainerThingy::MyContainerThingy(const MyContainerThingy &other)
: d(other.d)
{ }

MyContainerThingy::~MyContainerThingy()
{
}


I'm getting a compile time error:



compiling myclass.cpp (g++)
compiling mycontainerthingy.cpp (g++)
myclass.h:27: instantiated from here
/usr/include/qt4/QtCore/qshareddata.h:169: error: invalid use of incomplete type 'struct MyClassPrivate'
myclass.h:25: error: forward declaration of 'struct MyClassPrivate'
/usr/include/qt4/QtCore/qshareddata.h:170: error: invalid use of incomplete type 'struct MyClassPrivate'
myclass.h:25: error: forward declaration of 'struct MyClassPrivate'
/usr/include/qt4/QtCore/qshareddata.h:171: warning: possible problem detected in invocation of delete operator:
/usr/include/qt4/QtCore/qshareddata.h:171: warning: invalid use of incomplete type 'struct MyClassPrivate'
myclass.h:25: warning: forward declaration of 'struct MyClassPrivate'
/usr/include/qt4/QtCore/qshareddata.h:171: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.
*** Exited with status: 2 ***


I traced that back to when I call:


d->myclass = cls;

in my MyContainerThingy constructor;

Any ideas?

lni
12th June 2009, 22:49
You didn't read document enough...

You did

d->myclass = cls

but it has no operator=

Add the following methods



MyClass& operator=( const MyClass& rhs ) {
d = rhs.d;

return *this;
}

MyContainerThingy& operator=( const MyContainerThingy& rhs ) {
d = rhs.d;

return *this;

}



You don't need MyClassPrivate(const MyClassPrivate &other) or MyClass(const MyClass &other);

darkadept
12th June 2009, 23:20
Ok I added the operator= methods and the MyClass example I gave compiles, but my real-world example was still failing.

In the docs it says:


The copy constructor is not strictly required here, because class EmployeeData is included in the same file as class Employee (employee.h). However, including the private subclass of QSharedData in the same file as the public class containing the QSharedDataPointer is not typical. Normally, the idea is to hide the private subclass of QSharedData from the user by putting it in a separate file which would not be included in the public file. In this case, we would normally put class EmployeeData in a separate file, which would not be included in employee.h. Instead, we would just predeclare the private subclass EmployeeData in employee.h this way:

class EmployeeData;

If we had done it that way here, the copy constructor shown would be required. Since the copy constructor is trivial, you might as well just always include it.


So I put the copy-constructor back in and now it works.

lni
12th June 2009, 23:34
Sorry, I mean the private data copy ctor is not necessary, but the data object class needs both copy ctor and operator=

That is:

Needed:
MyClass( const MyClass& )
MyClass& operator=( const MyClass& )

Not needed:
MyClassPrivate( const MyClassPrivate& )

This applies to your container class too.

darkadept
12th June 2009, 23:40
Yup! That makes a whole lotta sense and everything compiles fine.

Thanks a ton! :D

darkadept
12th June 2009, 23:45
I also put this in the assignment operator to prevent self-assignment;

(as per http://www.parashift.com/c++-faq-lite/assignment-operators.html#faq-12.3)



MyClass &MyClass::operator=(const MyClass &rhs) {
if (this==&rhs) return *this; // <---- added this line
d=rhs.d;
return *this;
}

lni
13th June 2009, 01:53
I also put this in the assignment operator to prevent self-assignment;

(as per http://www.parashift.com/c++-faq-lite/assignment-operators.html#faq-12.3)



MyClass &MyClass::operator=(const MyClass &rhs) {
if (this==&rhs) return *this; // <---- added this line
d=rhs.d;
return *this;
}


That is a good practice. I don't sometimes do that since I know QSharedData will take care of it :)