views:

93

answers:

4

i'm having truble with deleting my template
my template and distructor :

template<class S, class T>  
class Consortium
{

private :

    map<const S, Node<T>*> m_consortiumMap;
    Heap<T>m_consortiumHeap;

public :

    ~Consortium();
    void Insert(const S key, T toAdd);
    void Update(const S key);
    void Remove(const S key);
    const T Top();
};

template<class S, class T>
Consortium<S,T>::~Consortium()
{
    m_consortiumMap.clear();
    delete &m_consortiumHeap.;
}

my heap and distructor :

template <class T>
class Heap
{
private :

    vector<Node<T>*> m_heapVector;

public :

    ~Heap();

    int parent(int i) const {return i / 2;}
    int left(int i) const {return 2 * i;}
    int right(int i) const {return 2 * i + 1;}
    void heapify(int index);
    Node<T>* extractMin ();
    void heapDecreaseKey (int index, Node<T>* key);
    void MinHeapInsert (Node<T>* key);
    Node<T>* ExtractNode(int index);
    Node<T>* top ()const {return m_heapVector[0];}

};  

template<class T>
Heap<T>::~Heap()
{
    for (int i = 0 ; i < m_heapVector.size() ; i++)
        m_heapVector.erase(m_heapVector.begin() + i);
}

and this is the object that holds the template, i'm having problems with him too

class Garage
{
    private :

        Consortium<string, Vehicle*> m_consortium;

    public :

        ~Garage() {delete &m_consortium;}
};

what's wrong here?

+1  A: 

You probably want to delete the objects pointed by the elements in the vector. Erase method doesn't do that, it just remove the pointer element from the vector, without destroying the pointed object. So you need (I presume) to delete the pointed object first, to avoid memory leaks. You cold do this:

for( vector<Node<T>*>::iterator iter = m_heapVector.begin(), endI = m_heapVector.end(); iter != endI; ++iter)
{
   delete *iter;
}

// m_heapVector.clean(); // Not necessary in destructor, since the vector will be destroyed anyway.

Using C++0x functions:

std::for_each( m_heapVector.begin(), m_heapVector.end(), []( Node<T>* node) { delete node; });

Also, use clear() method of the container (vector in your case) to remove all the elements.

Cătălin Pitiș
Or, if Boost is available, use a `ptr_vector`. Also, the same has to be done for `m_consortiumMap`, or he will have a memory leak.
Space_C0wb0y
Right. Still, I prefer the new future standard support for lambda functions... Much shorter, less dependencies :)
Cătălin Pitiș
A: 

Since m_consortiumHeap is a data member of your class (directly, not a pointer to it), you don't have to explicitly delete it. When the Consortium instance is destructed, it will automatically call the destructor of m_consortiumHeap for you.

Patrick
+2  A: 

This is wrong on its face:

delete &m_consortiumHeap;

You must only delete things that you allocated with new. m_consortiumHeap is part of the class and gets automatically allocated when the class gets allocated and automatically deallocated when the class gets deallocated. You cannot and must not explicitly delete it.

This may have the opposite problem:

m_consortiumMap.clear();

the contents of m_consortiumMap are pointers. I can't tell from the code you've shown, but if the nodes within the map are allocated by the Consortium class using new, they must be deleteed, otherwise you will leak memory. Clearing the map will only get rid of the pointers, it will not deallocate the memory that they point to. You must first iterate through the map and delete each element. While deallocation of the elements is important, clearing the map in the destructor is kind of pointless since the map itself will be destroyed immediately afterwards anyway.

This is just perplexing:

for (int i = 0 ; i < m_heapVector.size() ; i++)
    m_heapVector.erase(m_heapVector.begin() + i);

first of all, everything I said about m_consortiumMap applies also to m_heapVector: If the contents were allocated with new by the Heap class, you must delete them in the destructor. And erasing the pointers from the vector is pointless, not to mention that the above loop has a logic error in it. When you iterate over a container, you should use the iterators themselves, e.g.

for (std::vector<Node<T>*>::iterator i = m_heapVector.begin() ; i != m_heapVector.end() ; i++)

Also, std::vector, like std::map, has a clear() function, but like I said it's pointless to clear the vector in the destructor. What you really want to do is deallocate the elements (if necessary).

Tyler McHenry
@Tyler when i tried to use the vector iterator i got : vector iterator is not incrementable, that's why i used it like this. what's the right way to do it?
Roy Gavrielov
@Roy can you post exactly the error you received and tell us which compiler you're using? You absolutely can increment iterators, otherwise they'd be fairly useless.
Tyler McHenry
@Tyler yeah i asked it in a different question and got some good answers, here it is : http://stackoverflow.com/questions/3779227/why-is-this-vector-iterator-not-incrementable thanks.
Roy Gavrielov
+3  A: 

If you didn't use new to create an object you can't use delete to get rid of it.

Ferruccio