Problem with std::multimap

Question

Heya

I've got the following :

enum Type 
{ One = 0, Two};

class MySubClass
{
private:
MySubClass(); // prohibited
MySubClass(const MySubClass&); // prohibited
MySubClass & operator (const MySubClass&); // prohibited
public :
MySubClass(int x);
};

class MyClass 
{
MyClass(int x) : m_x(new SubClass(x)) 
{}
~MyClass() 
{ delete m_x; }
private :
MySubClass * m_x;
};

typedef multimap<Type, MyClass> my_multimap;
typedef pair<Type, MyClass> my_pair;

I'm trying to do the following :

my_multimap my_map;
my_map.insert(my_pair(One, MyClass(5)));

And I'm getting an unhandled exception result, the app is trying to read 0xfeeefeee etc.

What's going on? How can I fix this? Please note that this is a simplified case of what I'm dealing with;

Answer 1

You have to write a copy constructor.

What's happening is, that MyClass is copied by value the pointer is shared between the copies. Now when the objects are destroyed the pointer is deleted multiplie times.

Like this:

class MyClass 
{
MyClass(int x) : m_x(new SubClass(x)) {}
MyClass(const MyClass& myclass) : m_x(new SubClass(*myclass.m_x)) {}
~MyClass() { delete m_x; }
private :
MySubClass * m_x;
};

Obviously SubClass needs a copy constructor, too.

Answer 2

There's a rule of thumb, called the "Rule of Three": Whenever you have either a destructor or an assignment operator or a copy constructor, it is very likely you will need all three of them. Your code is no exception to this rule.

Think of what happens when objects of your type are copied. This

MyClass obj1;
MyClass obj2(obj1);

code will crash, too.

Answer 3

MyClass has no copy constructor defined. However, std::pair will need to make use of the copy constructor for MyClass. Presumably it is using MyClass's default copy constructor, which will give copy constructed objects copies of the pointer m_x. And when they get destroyed, you'll be facing multiple deletions.

Answer 4

As everybody mentioned, classes need a working copy constructor in order to be stored in any standard container. However, in this case MySubClass disables copying. This pretty much leaves you two options:

1) MyClass should be non-copyable too, in which case you'll have to store (smart) pointers in the multimap.

2) Copied MyClass instances should share the MySubClass instance. To implement this, the simplest is to replace the pointer member with boost::shared_ptr<MySubClass> or std::tr1::shared_ptr<MySubClass>. Doing so relieves you from the task of implementing a destructor, copy constructor and assignment operator.