Segfault while using Pimpl idiom with Qt types.

[darkadept]

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.H

class MyClassPrivate;

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

private:
  MyClassPrivate *d;
}

To copy to clipboard, switch view to plain text mode 

//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;
}

//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;
}

To copy to clipboard, switch view to plain text mode 

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.H

class MyContainerThingyPrivate;

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

private:
  MyContainerThingyPrivate *d;
}

To copy to clipboard, switch view to plain text mode 

//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;
}

//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;
}

To copy to clipboard, switch view to plain text mode 

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

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

To copy to clipboard, switch view to plain text mode 

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

Any ideas?

[lni]

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]

Ahhh. ok makes sense in the brain, now let's see what happens in implementation. I'll go try it right away. thanks

[darkadept]

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]

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]

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.H
 
class MyClassPrivate;
 
class MyClass {
public:
  MyClass();
  MyClass(const MyClass &other);
  ~MyClass();
 
private:
  QExplicitlySharedDataPointer<MyClassPrivate> d;
};

To copy to clipboard, switch view to plain text mode 

//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()
{ }

//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()
{ }

To copy to clipboard, switch view to plain text mode 

//MYCONTAINERTHINGY.H
 
class MyContainerThingyPrivate;
 
class MyContainerThingy {
public:
  MyContainerThingy(const MyClass &cls);
  MyContainerThingy(const MyContainerThingy &other);
  ~MyContainerThingy();
 
private:
  QExplicitlySharedDataPointer<MyContainerThingyPrivate> d;
};

//MYCONTAINERTHINGY.H
 
class MyContainerThingyPrivate;
 
class MyContainerThingy {
public:
  MyContainerThingy(const MyClass &cls);
  MyContainerThingy(const MyContainerThingy &other);
  ~MyContainerThingy();
 
private:
  QExplicitlySharedDataPointer<MyContainerThingyPrivate> d;
};

To copy to clipboard, switch view to plain text mode 

//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()
{  
}

//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()
{  
}

To copy to clipboard, switch view to plain text mode 

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 ***

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 ***

To copy to clipboard, switch view to plain text mode 

I traced that back to when I call:

d->myclass = cls;

d->myclass = cls;

To copy to clipboard, switch view to plain text mode 

in my MyContainerThingy constructor;

Any ideas?

[lni]

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;
 
}

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

  return *this;
}

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

  return *this;

}

To copy to clipboard, switch view to plain text mode 

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

[darkadept]

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]

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]

Yup! That makes a whole lotta sense and everything compiles fine.

Thanks a ton!

[darkadept]

I also put this in the assignment operator to prevent self-assignment;

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

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

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

To copy to clipboard, switch view to plain text mode 

[lni]

I also put this in the assignment operator to prevent self-assignment;

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

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

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

To copy to clipboard, switch view to plain text mode 

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