Problem with vector destructor

[ericeps]

The debugger shows these, the top 9. This occurs when I erase the last element in a vector.

std::vector<my_moving, std::allocator<my_moving> >::~vector
my_sight::~my_sight
__gnu_cxx::new_allocator<my_sight>::destroy
std::_Destroy<my_sight*, std::allocator<my_sight> >
std::vector<my_sight, std::allocator<my_sight> >::~vector
my_fighter::~my_fighter
__gnu_cxx::new_allocator<my_fighter>::destroy
std::vector<my_fighter, std::allocator<my_fighter> >::erase
my_enemies::action

As you can see, std::vector::erase is called for a vector of my_fighters.
The function that calls it:

void my_enemies::action()
{
	std::vector<my_fighter>::iterator ii=Fighters.begin();
	while(ii!=Fighters.end())
	{
		if(ii->Health<=0)
		{std::vector<my_fighter>::iterator tmp=ii;
		++ii;
		Fighters.erase(tmp);
		}
		else
		++ii;
	}
}

So, the element is erased if the Health is <= 0. The above debugger output comes up ONLY on the last element push_back()ed onto the vector.

The my_moving, my_sight, and my_fighter, showing how they fit together:

struct my_moving{
float x;
float z;
my_moving(float ax,float az){x=ax;z=az;}
};
struct my_sight{
std::vector< my_moving > movement;
std::vector< my_moving >::iterator iimove;
my_sight(std::vector<my_moving> amov){movement=amov;};

};

class my_fighter{
public:
int Health;
my_fighter(std::vector<my_sight> asightList)
std::vector<my_sight> SightList;
std::vector<my_sight>::iterator Sighted;
};

class my_enemies{
public:
std::vector::<my_fighter> Fighters;
void action();
};

Please help, I have been working through this issue for a few hours now. I have tried using lists instead of vectors, and tried compiling it on mac os x w/ xcode, and windows VS 2010. I'm basically stumped right now.

[guestgulkan]

This bit doesn't look safe

   
    {std::vector<my_fighter>::iterator tmp=ii;
    ++ii;
    Fighters.erase(tmp);
    }
 

If the object about to be erased is the last one (vector.end() -1), then ++ii
will make ii equal to vector.end().
After the erase, ii will be one past the new vector.end() - in other words ii is vector.end() + 1
- then when we get back at the top of the loop to make the test while(ii!=Fighters.end())
the test will be true because ii is not equal to Fighters.end() and we will
keep running the loop which will cause a vector out of bounds access.

** The erase function invalidates all currently held iterators - so you need to be careful here**

[krishnendu]

If an element of a vector is removed using erase method, all iterators are invalidated. That means you cannot use a previously initialized iterator to delete/access another element of the vector. Go through the following example.

void foo (void)
{
	int values[] = {1, 2, 3, 4, 5};
	vector<int> v (values, values + 5);
	// creates a vector with five elements

	vector<int>::iterator it = v.begin ();
	// The iterator points to the first element of the vector.

	v.erase (it);
	// First element is erased.
	// The second element becomes the first element.
	// All iterators are invalidated.
	// i.e. iterator 'it' cannot be used any more.

	it++;
	v.erase (it);
	// These two lines are buggy.
}

You are doing similar thing in line 7, 8 and 9 of your posted code. Refer to http://www.cplusplus.com/reference/stl/vector/erase/ for complete documentation on erase.

For this purpose std::list is the best choice. However, it is significantly slower than std::vector.

[ericeps]

Thanks for the replies, I did not know about the invalidating, but it makes sense since vector elements are contiguous.
Also, wanted to make sure that the code would be valid if the vector was turned into a list, similarly, I use code like this in many parts of my program:
[code]
std::list<aclass> MyList;
...
list<aclass>::iterator ii=MyList.begin();
while(ii != MyList.end())
{if(ii->x == 12)
alist.erase(ii++);
else
++ii;
}
[code]

[krishnendu]

This is perfect. But mind it, it is valid only for std::list.

[kbw]

Not quite. erase() returns the next iterator in the sequence:

list<aclass>::iterator ii=MyList.begin();
while(ii != MyList.end())
{
    if (ii->x == 12)
        ii = alist.erase(ii);
    else
        ++ii;
}