Delete pointer in a map

Question

Basically B is an object and A is an object manager, for each object created a new entry will be performed in m_myMap just in order to keep track of the number of objects and to be able to get a pointer of this object.

My question is: do I have to delete the pointers of B in m_myMap in A's destructor or will there be no memory leaks if m_myMap is automatically destroyed ?

A.h:

#ifndef A_H
#define A_H

#include "B.h"

class B;
class A
{
    public:
        A();
        ~A();
        int addB(B* myBPointer);
    private:
        std::map<int,B*> m_myMap;
}

#endif

B.h:

#ifndef B_H
#define B_H

#include "A.h"

class A;
class B
{
    public:
        B(A* givenA);
        ~B();
    private:
        A *m_myA;
        int m_id;
}

#enfif

B.cpp:

B::B(A* givenA)
{
    m_myA = givenA;
    m_id = m_myA->addB(this);
}
B::~B(){} // nothing here because m_myA is coming from another class and is destroyed in that class

A.cpp:

A::A();

A::~A()
{
    for(std::map<int,B*>::iterator it = m_myMap.begin();it != m_myMap.end();++it)
        delete it->second;// the program crashes
}

int A::addB(B* myBPointer)
{
    m_myMap.insert(std::pair<int,B*>(m_myMap.size(),myBPointer));
    return m_myMap.size()-1;
}

C.cpp and C.h:

#ifndef C_H
#define C_H

#include "A.h"
class C
{
    public:
        C();
        void myFunction();
    private:
        A* my_A;
}

#endif

C::C()
{
    my_A = new A;
}

void C::myFunction()
{
    B myObject(my_A);
}

`m_myMap` is a pointer, but your example doesn't show where you allocate the map. Either you've not posted a complete example or the problem is possibly that you don't allocate the map in the first place. — Sean, Nov 27 '14 at 16:27
Please stop using snippets. I removed them once already. They make absolutely no sense here. Click "Run code snippet" and see what you get. Is that what you were after? — , Nov 27 '14 at 18:27
Sorry for the snippets I have trouble getting my code into a post. — b0rn, Nov 30 '14 at 10:41

score 2 · Answer 1 · edited May 23 '17 at 12:22

One problem is that this:

m_myMap->erase(it);

invalidates it, so you can't safely use it afterwards. In general, to erase elements while iterating over a container, structure the loop as

for (auto it = map.begin(); it != map.end(); /* don't increment here*/) {
    delete it->second;
    it = map.erase(it);  // update here
}

In this case, since you're about to destroy the map anyway, there's no need to erase each element, only to delete the objects they point to.

You're also not following the Rule of Three, so it's possible that you could accidentally copy these objects, ending up with two trying to delete the same object. There's absolutely no reason for the map to be dynamically allocated, so replace that with an object member

std::map<int,B*> map;

There's still the problem that copying this will give two maps with pointers to the same B objects. If you don't have a good reason for dynamic allocation, then store the objects themselves in the map:

std::map<int,B> map;

If you do need dynamic allocation (perhaps because B is a base class, and the actual objects might be of various derived types), then either use a smart pointer (std::unique_ptr is probably suitable), or be very careful how you juggle the raw pointers.

In general, don't use pointers or new unless you really need to.

I think there is no need to erase an iterator in the loop: it can cause tree re-balancing which is just wasteful. The map will be deleted right after the loop. — roman-kashitsyn, Nov 27 '14 at 16:27
@roman-kashitsyn: True, I didn't read the code in full detail. I'll add a note of that. — Mike Seymour, Nov 27 '14 at 16:29

Delete pointer in a map

1 Answers1