std::map of user defined types crashes the program when retrieves a value

Question

I have an Item class, whici has a copy constructor, an assignment operator, copy constructor and compare constructor. Thus, it can be used in a std::map (right?).

I need to use my Item class in a std::map< Item, Item >. However it doesn`t work and crashes the program, and I have no ideea why.

This is my Item class:

#include <iostream>
#include <string>
#include <vector>
#include <map>

enum DataType {
    V_NULL,
    V_INT,
    V_FLOAT,
    V_DOUBLE,
    V_BOOL,
    V_STR,
    V_VECTOR,
    V_MAP
};

    // The Item can hold one of the types found in DataType enum
class Item {

    // Everything public at first
    public:
    // Constructor: default is NULL
    Item () {
    m_type = V_NULL;
    m_data = 0;
    }

    // Constructor:
    // template, so you can specify the data in the Item
    template<typename T>
    Item(T actualDataInItem, DataType type) {
        m_data = new Data<T>(actualDataInItem);
        m_type = type;
    }

    // Destructor
    ~Item () {
        delete m_data;
    }

    // Asignment operator:
    const Item& operator= (const Item& other) {
        // Check
        if (this != &other) {
            this->m_type = other.m_type;
        // free the memory m_data points to !!!
            delete m_data;
            if (other.m_data != NULL)
                m_data = other.m_data->clone();
        }
        return *this;
    }

    template<typename T>
    const Item& operator= (const T& newData) {
        delete m_data;
        if (newData != NULL) {
            m_data = new Data<T> (newData);
        }
        else {
            m_data = NULL;  // just for code reading
            this->m_type = V_NULL;
        }

        return *this;
    }


    // Copy constructor:
    Item(const Item& itemToCopy) {
        this->m_type = itemToCopy.m_type;
        this->m_data = itemToCopy.m_data->clone();
    }

    // Cast operator
    template<typename T>
    operator T () const {
        // dynamic_cast m_data to point to "an Item of type T"
        Data<T>* temp_data = dynamic_cast<Data<T>* > (m_data);
        return temp_data->m_dataOfAnyType;
    }

    // for the map
    bool operator< (const Item& other) const {
        return this->m_data < other.m_data;
    }

    // All Data inherits DataBase, so that you can
    // point to any Data<>
    class DataBase {
    public:
        // Pure virtual method for cloning the current data
        // Used when assignment operator is called with an Item argument
        virtual DataBase* clone () = 0;
        virtual ~DataBase () { }
    };

    // Data is the actual information carried in an Item.
    // It can be anything like a string, int, vector<Events> etc
    template<typename T>
    class Data : public DataBase {
    public:
        T m_dataOfAnyType;

        // Constructors:
        Data ();
        Data (T data) : m_dataOfAnyType(data) { }

        virtual DataBase* clone () {
            return new Data<T>(*this);
        }

    };

    // members of Item
    DataType m_type;
    DataBase * m_data;

};

Any ideas?

Answer 1

You have at least one problem here

// Asignment operator:
const Item& operator= (const Item& other) {
    // Check
    if (this != &other) {
        this->m_type = other.m_type;
    // free the memory m_data points to !!!
        delete m_data;
        if (other.m_data != NULL)
            m_data = other.m_data->clone();
    }
    return *this;
}

When you assign from an item where m_data is null you have a dangling pointer left and delete will be called another time on destruction or next assignement. To avoid this, you should add :

delete m_data;
m_data = NULL; // Assign there so that there is no dangling pointer even if clone() throws
if (other.m_data != NULL)
{
    m_data = other.m_data->clone();
}

I do not know if it is the root of your problem but it can be.

Answer 2

Neither the copy constructor nor the assignment operator for Item are correct. The copy constructor accesses the source m_data even when it is a null pointer. Also, it would be better to use initialization, rather than assignment (although it doesn't make a significant difference here):

Item::Item( Item const& other )
    : m_type( other.m_type )
    , m_data( other.m_date == nullptr ? nullptr : other.m_data->clone() )
{
}

The assignment operator has several problems; there are several different scenarios which will cause it to end up with a dangling pointer. The most obvious is if the other pointer is null; you don't set the target pointer to null. But also, if cloning throws an exception, you will be left with an unusable object (where even the destructor will cause undefined behavior).

A good hint that something is wrong here is the fact that you need to test for self assignment. If you need to test for self assignment, it's almost certain that you assignment operator isn't correct. The easiest way to correct it is to leverage off the copy constructor:

Item& Item::operator=( Item const& other )
{
    Item tmp;
    swap( tmp );
    return *this;
}

void swap( Item& other )
{
    std::swap( m_type, other.m_type );
    std::swap( m_data, other.m_data );
}

However, any solution which ensures that all operations which can fail occur before any modifications of your object would work, e.g.:

Item& Item::operator=( Item const& other )
{
    DataBase* new_data = other.m_data == nullptr
                         ? nullptr
                         : other.m_data->clone();
    delete m_data;
    m_type = other.m_type;
    m_data = new_data;
    return *this;
}

You'll also want to adopt this technique for the other assignment operators (which are similarly broken).

Answer 3

The copy constructor has an error

// Copy constructor:
Item(const Item& itemToCopy) {
    this->m_type = itemToCopy.m_type;
    this->m_data = itemToCopy.m_data->clone();
}

You are assuming that itemToCopy.m_data is not NULL, but nothing guarantees this that I can see. This is better

// Copy constructor:
Item(const Item& itemToCopy) {
    this->m_type = itemToCopy.m_type;
    this->m_data = itemToCopy.m_data ? itemToCopy.m_data->clone() : NULL;
}