std::map and _CrtIsValidHeapPointer error

Question

i try to use the std::map with

class DEMO {
    public:
        DEMO();
        virtual ~DEMO();
        DEMO &operator =(const DEMO &d);
        DEMO(const DEMO& d);
        BYTE* Arr() const;
    private:
        BYTE *m_array;
};

DEMO &DEMO::operator =(const DEMO &d) {
    memcpy(m_array, d.Arr(), 1);
    return *this;
}

DEMO::DEMO(const DEMO& d) {
    //call operator=
    *this = d;
}

const BYTE* DEMO::Arr() const {
    return m_array;
}

DEMO::DEMO() {
    m_array = new BYTE[1];
    }

DEMO::~DEMO() {
    if (m_array != 0)
        delete [] m_array;
}

class MyClass {
    private:
        typedef map<unsigned int,DEMO> t_mapType;
        t_mapType m_map;
        void Test();
}

void MyClass::Test() {
    DEMO myDEMO;
    m_map[1] = myDEMO;
}

if i call Test() within the class, i get the error assert _CrtIsValidHeapPointer...
i checked with breakpoints and i see, during assingment (m_map[1] = myDEMO;) the destructor of DEMO gets called and i get the error on delete [] m_array; - how i make this running ?

Answer 1

The copy constructor and assignment operators are wrong. You need to allocate memory for m_array before doing memcpy. When you try to insert a DEMO object into the map using m_map[1] = myDEMO; a copy of the myDEMO is being created. To create a copy the DEMO class's copy ctor is used. Since the memory for m_array is not allocated while doing this copy, the memcpy call is crashing.

Answer 2

It's almost always wrong to base the copy ctor implementation on the assignment operator's. An assignment operator has to tear down the object's old state and build a new state. The copy ctor only has to build a new state.
If you want to do it this way, your copy ctor first needs to build a default state for the assignment operator to tear down. Of course, that's wasting resources.

That said, actually you should use the copy-and-swap idiom to base your assignment operator on your dtor and copy ctor instead. See here for a more thorough explanation of the above.

Answer 3

Use a std::vector to manage your BYTE*.

class DEMO {
    public:
        DEMO();
        virtual ~DEMO();
        //DEMO &operator =(const DEMO &d);
        //DEMO(const DEMO& d);
        const std::vector<BYTE>& Arr() const;
    private:
        std::vector<BYTE> m_array;
};

// Doesn't even need to be implemented by us anymore
//DEMO &DEMO::operator =(const DEMO &d) {
//    memcpy(m_array, d.Arr(), 1);
//    return *this;
//}

// Again, doesn't need to be implemented
//DEMO::DEMO(const DEMO& d) {
//    //call operator=
//    *this = d;
//}

// Just changed to return const vector&.
const std::vector<BYTE>& DEMO::Arr() const {
    return m_array;
}

// Changed to use initializer list
DEMO::DEMO() 
    : m_array(1) {
    // Old code: m_array = new BYTE[1];
}

// Doesn't need any code by us
DEMO::~DEMO() {
    //if (m_array != 0)
    //    delete [] m_array;
}

Answer 4

i changed my code to use pointer to object instead of object for the value part in the map:

typedef map<unsigned int,DEMO*> t_mapType;

so i can assing as follow:

m_map[1] = new DEMO();

and i have to use a own free-mem method:

void DEMO::Clear() {
    if (m_map.size() > 0) {
        for (t_mapType::const_iterator it = m_map.begin(); it != m_mp.end(); ++it) {
            if (it->second != 0)
                delete it->second;
        }
        m_map.clear();
    }
}