# Cleaning up a vector of pointers?

This topic is 2082 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Hey everybody. I've run into a snag, and I can't quite figure it out. I'm making a maze game with c++ and SFML.

My problem is with my Map class. My map class parses a text file (which is a text version of the map grid), and reads all 1s as Walls and all 0s as blank spaces. The 0s are ignored, and for each 1 that it encounters, it takes the coordinates of the '1' and creates a pointer to a Wall object with those coordinates, and adds it to my _mazeMap vector.

 vector _mazeMap; _mazeMap.push_back(new Wall(x, y)); 

is the gist of it.

My problem is cleaning up the memory. In my Map class' destructor I figure I need to delete all the pointers that were created in the vector, correct? So this is the code I wrote to try and do it:

 Map::~Map() {     std::vector::iterator iter = _mazeMap.begin();          for (iter = _mazeMap.begin(); iter != _mazeMap.end(); ++iter)     {         delete *iter;     } } 

However, every time I run the game, when I shut it down, and the Destructor gets run, I get this error:

I have no idea what I'm doing wrong. Any help? Thanks so much.

##### Share on other sites
What line is it actually crashing at?
Unless one of these pointers already got deleted somewhere else, there is nothing wrong with your loop.
However, it is always a good idea to also empty the vector when you are done... but this shouldn't be a problem here.

 Map::~Map() { std::vector::iterator iter = _mazeMap.begin(); for (iter = _mazeMap.begin(); iter != _mazeMap.end(); ++iter) { delete *iter; } _mazeMap.clear(); } 

##### Share on other sites
You have confirmed that commenting out only the 'delete *iter;' line removes the crash?

In that case, I suspect you are either doing double delete, or the 'this' pointer in Map dtor is bad to start with. Check that you don't make value copies of the Map object, or that if you do, you have proper copy-ctor and assignment operators defined, since otherwise making copies of _mazeMap containers will cause duplicates of the pointers to be created, leading to a double delete on the long run.

##### Share on other sites
It gives me the bad access error on the
delete *iter
line.

I'm still not sure what's wrong. It's still happening, and I can't really understand it.
Here's my code that Loads the Map/Vector in case that would help at all.

 // Loads the map from a text file. void Map::Load() { // Opens the file. std::ifstream loadedFile; loadedFile.open("images/level.txt"); // Variable that will hold each line for parsing. std::string line; // Counts how many rows have been completed. int rowcounter = 0; // Makes sure the file has opened. if(loadedFile.is_open()) { // Remains true until the file has ended. // Loops through each line. while(std::getline(loadedFile, line)) { // Once the line has been put into the string variable, // this will go through each character an check for walls. for (int i = 0; i < LEVELWIDTH; i++) { // If a wall (a '1' character in the text file) is found // then this will put it into the vector. if (line == '1') { _mazeMap.push_back(new Wall(i, rowcounter)); } } // After the line has been parsed, this iterates the counter. rowcounter++; } } } 

Edit:

Yes, commenting out the (delete *iter) line causes the bug to disappear.
As far as I'm aware, I don't make any copies of _mazeMap or Map objects. I have a single static Map object in my Game Global, and that's it. Edited by bean.

##### Share on other sites

It's empty. Is that where my problem lies? I didn't give it much thought since it just has a few member variables and a draw function.

##### Share on other sites
Can you print out 'printf("0x%p", *iter);' just before calling 'delete *iter;' to confirm that double-delete doesn't occur?

Also, going for more exotic causes, can you check that you use the matching implementations of operator new and delete, and e.g. don't straddle the calls across module boundaries, or don't have any custom (debug-mode?) new/delete implementations going on.

##### Share on other sites

Can you print out 'printf("0x%p", *iter);' just before calling 'delete *iter;' to confirm that double-delete doesn't occur?

Also, going for more exotic causes, can you check that you use the matching implementations of operator new and delete, and e.g. don't straddle the calls across module boundaries, or don't have any custom (debug-mode?) new/delete implementations going on.

This is the output I get when that printf statement executes: 0x0xc00000001012f520
And that's all I get. I'm not extremely experienced at programming, just enough to know what I'm doing, I'd say. However, I'm not sure how to go about checking my implementations of new and delete. I haven't overloaded them, if that's what you're asking? How should I go about checking what implementations of new and delete I'm using? Thanks for the help.

Edit:
I commented out the (delete *iter) line with the printf statement still inserted. It gave me a ton of memory addresses. So, for some reason the program is crashing as soon as it tries to delete the first pointer. Edited by bean.

##### Share on other sites
Instead of "delete *iter; ", try this:
if (*iter) {delete *iter; *iter = NULL;}

Does the crash go away? If so you've definitely got double-delete, if not then you've got something a little more insidious.

##### Share on other sites

Instead of "delete *iter; ", try this:
if (*iter) {delete *iter; *iter = NULL;}

Does the crash go away? If so you've definitely got double-delete, if not then you've got something a little more insidious.

The crash did not go away when I replaced the code.

Here's my debugger output if that sheds light on anything? I'm just lost. I can't figure out what I'm doing wrong.

 GNU gdb 6.3.50-20050815 (Apple version gdb-1708) (Thu Nov 3 21:59:02 UTC 2011) Copyright 2004 Free Software Foundation, Inc. GDB is free software, covered by the GNU General Public License, and you are welcome to change it and/or distribute copies of it under certain conditions. Type "show copying" to see the conditions. There is absolutely no warranty for GDB. Type "show warranty" for details. This GDB was configured as "x86_64-apple-darwin".tty /dev/ttys000 warning: Unable to read symbols for @executable_path/../Frameworks/sfml-audio.framework/Versions/A/sfml-audio (file not found). warning: Unable to read symbols from "sfml-audio" (not yet mapped into memory). warning: Unable to read symbols for @executable_path/../Frameworks/sfml-graphics.framework/Versions/A/sfml-graphics (file not found). warning: Unable to read symbols from "sfml-graphics" (not yet mapped into memory). warning: Unable to read symbols for @executable_path/../Frameworks/sfml-network.framework/Versions/A/sfml-network (file not found). warning: Unable to read symbols from "sfml-network" (not yet mapped into memory). warning: Unable to read symbols for @executable_path/../Frameworks/sfml-system.framework/Versions/A/sfml-system (file not found). warning: Unable to read symbols from "sfml-system" (not yet mapped into memory). warning: Unable to read symbols for @executable_path/../Frameworks/sfml-window.framework/Versions/A/sfml-window (file not found). warning: Unable to read symbols from "sfml-window" (not yet mapped into memory). warning: Unable to read symbols for @executable_path/../Frameworks/SFML.framework/Versions/A/SFML (file not found). warning: Unable to read symbols from "SFML" (not yet mapped into memory). [Switching to process 49627 thread 0x0] 0x0x1000000000000000sharedlibrary apply-load-rules all Warning: the current language does not match this frame. Current language: auto; currently c++ (gdb) 

Update: Does this help at all? It's a picture of my debug area, and I think it's showing what's going on underneath the iterator. I'm not sure I particularly understand it though.

Update 2:
Could my issue have anything to do with the fact that class Wall extends VisibleGameObject? I have a blank virtual destructor in VisibleGameObject and a blank destructor in Wall. Edited by bean.

##### Share on other sites
Use your debugger and step through the deletion.
Lots of stuff happens when you free memory. Not only does the destructor of Wall get called, but also the destructors of any object that Wall contained and VisibleGameObject.

Post the class definition of both Wall and VisibleGameObject

Edit: Does GDB not automatically break execution when it crashes?? The debugger should catch this. What IDE are you using? Edited by jnmacd

##### Share on other sites

Use your debugger and step through the deletion.
Lots of stuff happens when you free memory. Not only does the destructor of Wall get called, but also the destructors of any object that Wall contained and VisibleGameObject.

Post the class definition of both Wall and VisibleGameObject

Edit: Does GDB not automatically break execution when it crashes?? The debugger should catch this. What IDE are you using?

I use xcode (developing on a macbook). I don't have much experience with my debugger, so I'm going to go do some research and figure out how to properly use it. Here are the 2 class definitions.

 class Wall : public VisibleGameObject { public: // Functions explained in implementation. Wall(int x, int y); ~Wall(); int GetCellX(); int GetCellY(); void Draw(sf::RenderWindow& rw); private: // X and Y position member variables. int _cellPosX; int _cellPosY; }; 

 class VisibleGameObject { public: VisibleGameObject(); virtual ~VisibleGameObject(); // Loads an image for the object. virtual void Load(std::string filename); // Draws the object to the screen. virtual void Draw(sf::RenderWindow& window); // Updates the object's position. virtual void Update(float elapsedTime); // Sets the position of the sprite. virtual void SetPosition(float x, float y); // Returns a Vector for the position of the sprite. virtual sf::Vector2f GetPosition() const; // Boolean value for the image/sprite being loaded virtual bool IsLoaded() const; protected: // Provides access to the sprite sf::Sprite& GetSprite(); private: // Member variables sf::Sprite _sprite; sf::Image _image; std::string _filename; bool _isLoaded; }; 

##### Share on other sites
If you want to avoid manual deletion of the pointers you could use a vector of smart pointers instead of raw pointers. C++11 offers you std::shared_ptr.

##### Share on other sites

If you want to avoid manual deletion of the pointers you could use a vector of smart pointers instead of raw pointers. C++11 offers you std::shared_ptr.

Seeing as OP has commented multiple times they are not very strong with programming, this may be pointing them to a completely different direction, especially when I am under the impression that if they figure out this problem, they will learn more about memory management than they will from the moving to the new C++ standard.

Here are the 2 class definitions.

Can you also post the Wall and VisibleGameObject constructors and destructors?

##### Share on other sites

Seeing as OP has commented multiple times they are not very strong with programming, this may be pointing them to a completely different direction, especially when I am under the impression that if they figure out this problem, they will learn more about memory management than they will from the moving to the new C++ standard.

You are totally right! The hard way is sometimes better (that's why we c++ nerds are often better programmers ;)

##### Share on other sites
Thanks for all the help so far! Yeah, I'm just trying to tackle this right now, cause if I can get this to work, then my game (final project for my 1st programming course) will be done. Here's the constructors and destructors of Wall and VisibleGameObject, and an extra function or two that might be necessary to see what's going on.

 // Loads the wall from an image. Sets its position. Wall::Wall(int x, int y) : _cellPosX(x), _cellPosY(y) { Load("images/Wall.png"); SetPosition(_cellPosX * Game::CELL_SIZE, _cellPosY * Game::CELL_SIZE); assert(IsLoaded()); } Wall::~Wall() { } 

 // When an object is created, it is not yet loaded. VisibleGameObject::VisibleGameObject() { _isLoaded=false; } VisibleGameObject::~VisibleGameObject() { } // Loads an image and sets it as the sprite. void VisibleGameObject::Load(std::string filename) { // Handles the image not loading. if(_image.LoadFromFile(filename) == false) { _filename = ""; _isLoaded = false; } // Sets the image for the sprite. else { _filename = filename; _sprite.SetImage(_image); _isLoaded = true; } } 

Edit: Sorry about that. I'm not sure what happened. Code fixed. Edited by bean.

##### Share on other sites
1. Your last post isn't complete. ( I realized a little late you said your dtors were empty, lol )

This might help a little. It seems as though something is accessing some allocated memory after it was released, so your delete/clean up steps are most likely correct, but somewhere down the line something ELSE is trying to access memory you've already freed. Since commenting out the delete *iter stopped the error from happening, the culprit is most likely _mazeMap.

3. Since I am not familiar with xcode or GDB debugger, what is _mazeMap's and iter's REAL definitions? You have
vector _mazeMap; std::vector::iterator iter = _mazeMap.begin();
which don't seem like correct syntax.
vector< Wall * > _mazeMap; std::vector< Wall* >::iterator iter = _mazeMap.begin();
It's better to post *exactly* what you have, instead of cleaning it up for a post.

I would also try the _mazeMap.clear(). If you don't, you basically have _mazeMap full of garbage pointers.

4. Get familiar with breakpoints. They will be your best friend in situations like this.

##### Share on other sites
I learned a good deal about debugging today, a lot more than I knew before. However, all I was able to gather was that something was releasing my pointers before the destructor of Map was being called. So, I just took out the destructor, and used Instruments to run it and check it for Memory Leaks. It didn't detect any, so, I'm not sure what's happening. I tried using NSZombie to help, but it didn't get me too far. Using breakpoints, I noticed, that right before delete *iter is called, the value of *iter is non existant, as it does not have the X and Y values that a *Wall should have.

Is there any way that pointers could get released before calling delete on them? I'm not using smart pointers. Unless my pointers are magically smarter than I thought.

##### Share on other sites
I would set breakpoints on every line of code that has _mazeMap on it, and make sure it has what you think it does on every line.

And is this code EXACTLY what you have in your Map destructor? (My guess is no, if that is the case, post the actual dtor.)
 Map::~Map() { std::vector::iterator iter = _mazeMap.begin(); for (iter = _mazeMap.begin(); iter != _mazeMap.end(); ++iter) { delete *iter; } } 

##### Share on other sites
Not to mention that you better make sure that there is absolutely no location in your code that creates a copy of the map or calls a function where you pass it by value. It at any point you create a temporary copy of that map, all your pointers will be invalid and your walls deleted.

If that is the case, it might be time to google for the Rule of Three.

Did you place a breakpoint in the destructor and make sure it only gets called once? Debug output might actually work better, as some debuggers are notoriously bad with breakpoints in destructors.

If in doubt, place a breakpoint in the destructor of your Wall class and check the call stack when you hit it to see where it's getting called.

Things don't get magically deleted on their own. A more far fetched explanation would be writing out of bounds and screwing up the internal pointer of the vector. Edited by Trienco

##### Share on other sites
Could it be his iterator itself is trying to access what got deleted?

##### Share on other sites
I don't think you need to use 'new Maze(int x, int y)' to allocate memory for a wall, because the vector automatically allocates memory for an object when you use push_back().

//Create a local variable of type Wall before you push back
Wall myWall(_some_X_, _some_Y_);

//Push back myWall without using new
_mazeMap.push_back(myWall);

after this, you don't need to delete the pointers, just use _mazeMap.pop_back() until the vector is cleared.

And, there's a thing with vectors, that when a vector size is adjusted (you push_back more objects than the reserved size), all the objects on the vector are reallocated, so the old adresses will be lost. I can't explain very well, but if you read about vectors, you'll know more.

Hope i helped.

##### Share on other sites
Whenever I hear this same problem description, my first advice is to google for "C++ rule of three" -- the first hit probably has the solution to your problem.

As 3333360 pointed out, you could also try switching to using a value container instead of a pointer container.

##### Share on other sites
In order to make sure the destructor of Map is called only once, maybe you could put a break point on its first line to check this?
Does the program crash when you use clear() on your vector<> at the end of the destructor ?
My 2 cents

##### Share on other sites

Could it be his iterator itself is trying to access what got deleted?

No. It could be him attempting to access an iterator after he's invalidated it, but iterators by themselves will not do anything.

after this, you don't need to delete the pointers, just use _mazeMap.pop_back() until the vector is cleared.

or just call _mazeMap.clear();

And, there's a thing with vectors, that when a vector size is adjusted (you push_back more objects than the reserved size), all the objects on the vector are reallocated, so the old adresses will be lost. I can't explain very well, but if you read about vectors, you'll know more.

That has nothing to do with the memory he's allocated using new.

My suggestion would be to use boost::ptr_vector if you're going to insist on doing your own memory management. Edited by Washu