STL map and dynamic memory

sup guys
Just joined the forums. Im having some STL problems with deleting dynamic memory.
The problem happens when I get to the last element left in the map. My guess is im not revalidating the iterator right.
Any Ideas?

Code:

---

typedef map <int, AnimFile*> ANIM_FILE_MAP;   

ANIM_FILE_MAP::iterator fileItr;
for (fileItr = currNode->nodeAnimMap.begin();
 fileItr != currNode->nodeAnimMap.end();)
{       
    AnimFile* tempInfo = fileItr->second;
    delete [] tempInfo->anims;
    delete [] tempInfo->localOffsets;
    delete tempInfo;

    fileItr = currNode->nodeAnimMap.erase(fileItr);
}

---

That shouldn't even compile. map<> does not have a member function erase() that takes an iterator and returns an iterator.

You don't need to call erase(). Call the clear() method on the container after iterating over all the elements.

gg

Wow!! your right according to SGI, yet visual studios library says yes. Thats messed up. In either case I got rid of the erase and used clear() after the loop yet it still hangs up when I reach the last element left.

I spent a few minutes trying to grok what's going on there, and I'm thinking that this might be dangerously confusing.

To start you:
- dynamically allocate a new object of type AnimFile
- store the pointer to the object in a map
- Then you dynamically allocate memory for its members, anims and localOffsets

So after you do what needs to be done, you have to remember to:

- Loop through the map
- deallocate all the member's memory
- deallocate the AnimFile object
- tip-toe around a map full of dangling pointers

The class encapsulation lets you hide this detail. For example, you could set anims and localOffsets using functions, taking care of the allocation inside the class. You could then leave the deallocation to the destructor (guaranteeing the memory is always freed). If you needed to clear the data at some arbitrary point, you could implement a clear() function (which would incidentally make up the meat of your destructor as well).

Another idea, which could limit functionality (polymorphic behavior), is to create a map full of AnimFile objects, instead of pointers to AnimFile objects. As soon as the map goes out of scope (or is cleared by you), all the objects would be automatically destroyed. By using the iterators, you get pointer like behavior. And in a map, adding or removing elements does not invalidate any other pointers.

Maybe some of that will be helpful. Or maybe it doesn't at all fit with what you have.

I can't see your problem, but if you post more code (enough I could compile) I could be of more direct help. If not perhaps someone else will spot the problem.

One other thing, your idea of walking through the nodes in the body of the for loop instead of it's definitions seems kinda backwards. Was there a particular reason behind it?

Personally I'd say use smart pointers (NOT std::auto_ptr, but boost::shared_ptr) instead of raw pointers inside the map and leave the memory handling to them.

Oh, I just realized that AH_Tze is right, there's a definite lack of encapsulation there. AnimFile should indeed delete its contained pointers by itself in the destructor.

boost smart pointers are good, but deleting memory is possible on std::vectors as well, and this is how I do it:

Code:

---


#include    <algorithm>    //Otherwise anything that uses these has to manually include this
template<class T>
struct VectorDeleter
{       
    void operator()(T * & obj)    //reference to a pointer, not a pointer to a reference
    {           
        if(obj)       
        {           
            delete obj;               
            obj = NULL;    //I don't know if this actually does anything...hmm   
        }   
    }
};

//Deleter defined in GAMEEXPORT.H right now
#define    DELETE_MEMORY_AND_ERASE(a,t)    std::for_each(a.begin(),a.end(),VectorDeleter<t>());    a.clear();
#define    DELETE_INDEXED_MEMORY_AND_ERASE(vec,type,ptr)    std::for_each(ptr,ptr+1,VectorDeleter<type>());    vec.clear();

---

The algorithm header file is for std::for_each

the first macro deletes all of the dynamic memory on the vecotr, the second only deletes the memory at one index into the vector (so you can delete the i'th value in the array).

It's not immediately clear to understand, and I had some help generating this code so it's not like I 100% wrote this on my own.

You have macros and no exception safety. I prefer smart pointers...

Also, the if(obj) is useless.
And the second macro is hard to use. Given an index, I have to do:

Code:

---

DELETE_INDEXED_MEMORY_AND_ERASE(myvec, int*, myvec.begin()+index);

---

When I could do

Code:

---

delete myvec[index];
myvec[index] = 0;

---

which is shorter AND more readable.

Thanks for the replies, especially Ah_Tze. AnimFile is acually a struct which is why I need to loop through the map and delete everything before calling clear. But you make a good point about encapsulating it.

>>yet visual studios library says yes.
Just a quick comment: I'm not sure what version of visual studio you have, but on mine it adds a disclaimer:
"Note that this return value is nonstandard behaviour."
;)

How is the if(obj) useless? You may be right, but I dont' exactly understand why (maybe the syntax should be different, i.e the * and & should be switched, but I haven't had problems with it). Isn't that the equivalent of:

Code:

---

if(MyVec[0])
{
delete MyVec[0];
MyVec[0] = 0;
}

---

because

Code:

---

std::vector::operator[]

---

returns a reference, and doing

if(MyVec[0])

is checking whether the pointer is already nulled through a reference.

One other thing:
When I want to deallocate memory from a std::vector I typically also want to get rid of the raw pointer. So, doing what you do, you have to add a line of code something like:

Code:

---

std::vector<type>::iterator ptr = MyVec.begin();
delete MyVec[0]; //std::vector::operator [] returns a reference

MyVec[0] = 0;

MyVec.erase(ptr);

---

erase in itself doesn't delete memory, just raw pointers. So, the code I showed doesn't delete memory that's already nulled, and it also can get rid of pointers all in one step.

>>is checking whether the pointer is already nulled through a reference.
Indeed, and that's why it's bad ;) The C++ language guarantees that deleting a NULL pointer will do nothing, so you're just wasting CPU cycles by adding an unnecessary branching statement.

>>When I want to deallocate memory from a std::vector
Just like to point out, the OP is dealing with a std::map..


Ted, have you updated your code (and are you still having the problem)? It would be helpful to see what you're working with at the moment

Isn't the process for handling dynamic memory pretty much the same for each stl type (it seems it is which is why I thought my code might help).

I also just took out that if statement :)

Well, you do have to delete the dynamic memory etc. as you would in a vector. On the other hand, as Codeplug mentioned you can't really erase elements in a std::map like you can in a std::vector.. so you'd probably just iterate through the map deleting all dynamic memory, and then when you're done you'd call clear() to remove all elements of the map. And that's where it's a little bit different :) Although, apparently, that isn't where the problem lies in Ted's code.

Quote:

---

Originally Posted by Hunter2

...On the other hand, as Codeplug mentioned you can't really erase elements in a std::map like you can in a std::vector...

---

?

from SGI ( http://www.sgi.com/tech/stl/Map.html )

Code:

---

void erase(iterator pos)
size_type erase(const key_type& k)
void erase(iterator first, iterator last)

---

Codeplug said there wasn't an erase function that returned an iterator (which makes sense).

Thanks for the replies, Yeah its all fixed. The AnimFile* wasnt really necessary as value worked just fine. That and the use of the string class solved problems.

ohh and yes just irtating through the map deleting all dynamic memory then callng clear() worked.

@ AH_Tze:

Indeed, that's what I meant (although perhaps I wasn't clear). List and vector (probably deque too) all have mechanisms by which you can erase an element and continue looping; with map, as far as I can tell, if you erase an element then there is no way to get an iterator for the next element and therefore you must start over from the beginning again. In this case, it seems that would be a simple thing to do - since every element is deleted, the 'next' element is always the first element, so begin() should suffice to provide you with suitable iterators.

Come to think about it, my post has less and less meaning every time I read through it :D