Cleaning up a vector of pointers?

Started by
26 comments, last by Storyyeller 11 years, 11 months ago
Could it be his iterator itself is trying to access what got deleted?
Advertisement
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().

Try this instead:

//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.
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.

Stephen M. Webb
Professional Free Software Developer

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

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.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

Thank you everyone for the help. This has been an awesome learning experience for me. The Rule of Three was my problem, specifically, I was passing my Map object by reference to my Player object to check for collisions.



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.


This. After a whole lot of hair pulling, I think this was it. I knew that I wasn't making any copies of my map object, but I didn't know that passing it by reference would be an issue. I'm not sure why I didn't think to mention that sooner.

So, I'm going to do some more research tomorrow. I understand how to write Destructors and Assignment operators, I just have to learn more about copy constructors.

Here was the problem code:



if (update)
{
player.Update(_map);
}




void Player::Update(Map &mp)
{

// These variables will record the distance that the player is going to move.
float moveX = 0.0f;
float moveY = 0.0f;

// Movement based on keypresses.
if(Game::GetInput().IsKeyDown(sf::Key::Left))
{
moveX--;
}
else if(Game::GetInput().IsKeyDown(sf::Key::Right))
{
moveX++;
}
else if(Game::GetInput().IsKeyDown(sf::Key::Down))
{
moveY++;
}
else if(Game::GetInput().IsKeyDown(sf::Key::Up))
{
moveY--;
}

// If the new destination is not a wall, move the player.
if(mp.IsPassable((_cellPosX + moveX), (_cellPosY + moveY)))
{
GetSprite().Move(moveX * Game::CELL_SIZE, moveY * Game::CELL_SIZE);
_cellPosX += moveX;
_cellPosY += moveY;
//return;
}
}




bool Map::IsPassable(int x, int y)
{
// Iterates through the vector.
std::vector<Wall*>::iterator iter;
for (iter = _mazeMap.begin(); iter != _mazeMap.end(); ++iter)
{
// If the x and y coordinates of the tile are equal to a wall's x and y coords
// then the tile is not passable.
if ((*iter)->GetCellX() == x && (*iter)->GetCellY() == y)
{
return false;
}
//std::cout << std::endl << "X: " << (*iter).GetCellX() << std::endl << "Y: " << (*iter).GetCellY() << std::endl;
}
return true;
}
Passing the map by reference shouldn't create any copies. You could try to derive your map from boost::noncopyable or just make your assignment and copy constructor private. Your compiler should end up telling you where the problem is coming from by complaining about it.

Considering the nature of your map, making it non-copyable is probably making more sense than creating absurdly expensive assignment and copy constructors.
f@dzhttp://festini.device-zero.de
I second the recommendation to switch to smart pointers. As a beginner you shouldn't do everything the hardest way possible just because you feel like you need to learn everything at once. And besides, even once you learn about manual memory management, you'll still be using smart pointers 99% of the time anyway. Generally, the only times you should use raw pointers are where performance is critical (like writing a library), or you have to interface with a library that uses raw pointers (which is most of them).
I trust exceptions about as far as I can throw them.

This topic is closed to new replies.

Advertisement