34 replies to this topic

[Servant of the Lord]

Just so I'm clear, the iterator isn't invalidated by pop_back(), except when the iterator is pointing at the last element, right?

 

std::vector::pop_back() - The end iterator and any iterator, pointer and reference referring to the removed element are invalidated.
Iterators, pointers and references referring to other elements that have not been removed are guaranteed to keep referring to the same elements they were referring to before the call.

So only if there is just one element left, then it will crash. I'm not seeing how it will crash on the first element.
 
So, would this work:

 

for(ZombieIterator it = zombies.begin(); it != zombies.end();)
{
	if(it->dead)
	{
		//Swap the value of the current zombie with the zombie at the back of the container.
		std::swap(*it, zombies.back());
		//Pop the back of the container.
		zombies.pop_back();
		
		if(zombies.empty())
		{
			break;
		}
	}
	else
	{
		++it;
	}
}

Or am I just digging myself deeper? 

Edited by Servant of the Lord, 21 January 2013 - 05:31 PM.

[Mercile55]

this will do the job

[TheChubu]

while ( it != zombies.end() )
{
    if  ( it->dead )
    {
        it = zombies.erase( it );
    }
    else
    {
        ++it;
    }
}

I remembered I had a similar issue before and using a while loop instead worked better

[belfegor]

Without iterators:

for(std::size_t i = 0; i < v.size(); ++i)
	{
		if(v[i].dead)
		{
			
			std::swap(v[i], v.back());
			v.pop_back();
			--i;
		}
	}

Edited by belfegor, 22 January 2013 - 04:32 AM.

[EWClay]

It's not a question of whether there's one element left or not, when you pop_back the last element the iterator is invalidated right before you compare it with end().

Now, I think a sensible implementation would not crash here (in contrast to the first version), but it should give an error in debug mode. You could get around it by special-casing the last element.

Or use indices, which don't have this problem.

[mypel16000]

@bleferog: That's it! I no longer count with the problem of various zombies being deleted at the same time. But one last thing, the swap function is not working..... only the last one is getting deleted.

[mypel16000]

anyone know why the swap isn't happening?

[belfegor]

...But one last thing, the swap function is not working..... only the last one is getting deleted.

If your vector is not holding pointers as elements then you must implement "proper" copy constructor and assignment operator, as implicit one might not do the job well depending on the data your zombie class is holding.

 

What not use shared_ptr?

 

class CZombie
{
private:
    bool dead;
    int health;
public:
    CZombie(int h) : dead(false), health(h) {}
    ...
    bool isDead() const { return dead; }
    void takeDamage(int damage)
    {
        health -= damage;
        if(health <= 0)
            dead = true;
    }
};
 
typedef std::shared_ptr<CZombie> sp_Zombie;
typedef std::vector<sp_Zombie> v_Zombie;
...
v_Zombie zombies;
...
    zombies.push_back( std::make_shared<CZombie>(100) );
    zombies.push_back( std::make_shared<CZombie>(120) );
    zombies.push_back( std::make_shared<CZombie>(100) );
    zombies.push_back( std::make_shared<CZombie>(150) );
...
    for(std::size_t i = 0; i < zombies.size(); ++i)
    {
        if(zombies[i]->isDead())
        {
            std::swap(zombies[i--], zombies.back());
            zombies.pop_back();
        }
    }

Notice that i moved decrement up and make it post-decrement, witch does the same thing, but have one line of code less.

[mypel16000]

I appreciate you giving me a solution to the problem, but this doesn't work on my C++.... y get the error 

expected initializer before '<' token| for the line typedef std::shared_ptr<CZombie> sp_Zombie;

Isn't there a simpler way of initialising a vector of pointers like maybe creating an array of objects and then using them to create a vector.... Please, I have got really far and I am just stuck on this silly little thing that I just can't seem to fix

[Servant of the Lord]

Are you actually needing pointers? Just use std::vector<Zombie> unless you actually need to, and use this code posted a couple posts ago.

[mypel16000]

actually your code doesn't work. It doesn't actually swap the objects, it just deletes the last one. I'm in need of a solution for your code

[belfegor]

Do you have included <memory> header? It is necessary for shared_ptr.

What compiler are you using?

 

If you want "pure" pointers then code is a bit different:

class CZombie
{
...
};
 
typedef std::vector<CZombie*> v_Zombies
 
class CZombieMgr
{
    v_Zombies zombies;
public:
...
 
~CZombieMgr()
{
    for(std::size_t i = 0; i < zombies.size(); ++i)
    {
        delete zombies[i];
    }
}
 
void addZombie(int h)
{
    zombies.push_back( new CZombie(h) );
}
 
void removeDeadZombies()
{
    for(std::size_t i = 0; i < zombies.size(); ++i)
    {
        if(zombies[i]->isDead())
        {
            std::swap( zombies[i--], zombies.back() );
            delete zombies.back();
            zombies.pop_back();
        }
    }
};
...

[belfegor]

@moderators

Sorry for double post, but if i go to EDIT then it somehow messes my code up.

 

@mypal1600000

Can you post relevant bits from your code? it will be easier to help you.

[mypel16000]

No! It won't be needed as my code has been fixed. Thanks to all of you who, by discussing together, have found the solution to the problem.

[Mercile55]

Without iterators:

for(std::size_t i = 0; i < v.size(); ++i)
	{
		if(v[i].dead)
		{
			
			std::swap(v[i], v.back());
			v.pop_back();
			--i;
		}
	}

Remember that modification of the for-loop index inside the loop is one bad programming practice. It usually leads to infinite loops and computer hang ups.