• Advertisement
Sign in to follow this  

VC 2005 express and the stl vector class

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

If you intended to correct an error in the post then please contact us.

Recommended Posts

I just moved from VC6 to the free VC8. I have downloaded and installed the platform sdk. I believe I have configured everything correctly. I can create a sample win32 program and it runs fine. Now I am trying to recreate some of my old programs from VC6. I'm running into a problem with the vector class. The game runs fines, but if I close the game it crashes. I've narrowed the problem down to the function that cleans up all the sprites. I store the sprites in a vector. Here is the function
void GameEngine::CleanupSprites()
{
  // Delete and remove the sprites in the sprite vector
  vector<Sprite*>::iterator siSprite;
  for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); siSprite++)
  {
    delete (*siSprite);
    m_vSprites.erase(siSprite);
    siSprite--;
  }
}
Is there a problem with accessing the first member in a vector? The program seems to crash only when the 1st member is deleted. I am using cpp, and this program worked fine in VC6. Any ideas?

Share this post


Link to post
Share on other sites
Advertisement
Your code is non-kosher iterator usage. Decrementing a vector iterator that points to the first element is not valid. Consider calling delete on all the elements and then calling clear() rather than erasing as you delete.

Share this post


Link to post
Share on other sites
Quote:
Original post by cmptrgear
Doesnt erase also invalidate the iterator?

In a vector, yes, but it does return the next iterator.

Share this post


Link to post
Share on other sites
By the way, per SiCrane's suggestion [edit: first version would not call destructors, you could call them in a second pass, I suppose, but this is better]:
#include <algorithm>
#include <functional>

// ...

void Deleter( Sprite const * const p ) {
delete p;
}

void GameEngine::CleanupSprites()
{
std::for_each( m_vSprites.begin(), m_vSprites.end(), Deleter );
m_vSprites.clear();
}


[edit2: An even better solution would be to use boost::ptr_vector, or boost:shared_ptr with std::vector ]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
VC 6's STL implementation is horribly broken. Try STL Hack.

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
VC 6's STL implementation is horribly broken. Try STL Hack.


Of course if you had actually read his post you would realise that he's using VC8 now, and it's STL implementation is just fine.

Share this post


Link to post
Share on other sites
Quote:
Original post by joanusdmentia
Quote:
Original post by Anonymous Poster
VC 6's STL implementation is horribly broken. Try STL Hack.


Of course if you had actually read his post you would realise that he's using VC8 now, and it's STL implementation is just fine.


Or even just the title. And it's STLPort.

Share this post


Link to post
Share on other sites
Thanks for the advice. I altered the cleanup sprites function. Now it just deletes the sprite, then clears the entire vector after.

for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); siSprite++)
{
delete (*siSprite);
}
m_vSprites.clear();


Alternativly I could have done this right?

for_each(m_vSprites.begin(),m_vSprites.end(),delete (*siSprite) )



The other problem I had was during the sprite update function. When a sprite is marked as dying, it is removed from the vector inthe same manner as before.

// Kill the sprite
delete (*siSprite);
m_vSprites.erase(siSprite);
// siSprite--;
continue;


This worked only untill the 1st element in the vector was deleted. It was said that erase() returned the next iterator, I commented out the siSprite--, and now it works fine. Is this correct or am I just lucky it worked out this time?

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by localrob2
Alternativly I could have done this right?
*** Source Snippet Removed ***
time?

No.

Share this post


Link to post
Share on other sites
Quote:
Original post by Anonymous Poster
Quote:
Original post by localrob2
Alternativly I could have done this right?
*** Source Snippet Removed ***
time?

No.

As informative as that was, you might want to elaborate on that.

Share this post


Link to post
Share on other sites
C++ does not support arbitrary statements and expressions (ie: closures) to be passed to functions or assigned to variables.

You CAN pass the address of a function though, which is why the example using the Deleter function above is correct.

Share this post


Link to post
Share on other sites
Quote:
Original post by Alpha_ProgDes
Quote:
Original post by Anonymous Poster
Quote:
Original post by localrob2
Alternativly I could have done this right?
*** Source Snippet Removed ***
time?

No.

As informative as that was, you might want to elaborate on that.


I'm not the AP, but the reason is that for_each has the following signature:
template<class InputIterator, class Function>
Function for_each(InputIterator first, InputIterator last, Function f);


delete (*siSprite) won't return a "Function" also siSprite is not declared, this was the name used in your previous code but you can't do this (well you can get pretty close with Boost.Lambda) because it's not in this code.
jflanglois showed the correct approach if you use for_each.

Quote:
The other problem I had was during the sprite update function. When a sprite is marked as dying, it is removed from the vector inthe same manner as before.

<source>

This worked only untill the 1st element in the vector was deleted. It was said that erase() returned the next iterator, I commented out the siSprite--, and now it works fine. Is this correct or am I just lucky it worked out this time?

This is just luck, because the internal representation is continous you are unlucky enough that after erasing an iterator it will just point to the next element. The real solution is this:

void GameEngine::CleanupSprites()
{
// Delete and remove the sprites in the sprite vector
vector<Sprite*>::iterator siSprite;
for (siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); ++siSprite)
{
delete (*siSprite);
siSprite = m_vSprites.erase(siSprite); // erase returns next iterator
}
}

Share this post


Link to post
Share on other sites
That does make sense. But since the erase function now points to the next element, there shouldn't be a need for the For loop to increase the iterator right?
Thanks for all the help. I guess this maybe should have been moved to beginners.

Share this post


Link to post
Share on other sites
Quote:
Original post by localrob2
That does make sense. But since the erase function now points to the next element, there shouldn't be a need for the For loop to increase the iterator right?
Thanks for all the help. I guess this maybe should have been moved to beginners.

That is correct. It removes one out of every two elements at best (when you have an even amount of elements in the vector), and asserts/crashes/causes an undetected bug otherwise (when you have an odd number of elements in the vector). Just remove the incrementor from the for loop and it will work (for( siSprite = m_vSprites.begin(); siSprite != m_vSprites.end(); )).

This way is not very good performance-wise, however, since the vector is going to have to shift all the values back by one each time you call erase(), which is why SiCrane suggested you delete the pointers first, then clear() the vector (well, that, and the code is cleaner and easier to understand).

I still strongly suggest you look into boost::ptr_vector or boost:shared_ptr with std::vector.


jfl.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement