🔒 Locked

VC 2005 express and the stl vector class

Started by localrob2 Jul 7, 2006 at 7:46 PM 14 replies 1.8k views

Original Post

Jul 7, 2006 at 7:46 PM

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?

SiCrane

Jul 7, 2006 at 7:50 PM

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.

cmptrgear

Jul 7, 2006 at 10:57 PM

Doesnt erase also invalidate the iterator?

jflanglois

Jul 8, 2006 at 1:15 AM

Quote:
Original post by cmptrgear
Doesnt erase also invalidate the iterator?

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

jflanglois

Jul 8, 2006 at 1:25 AM

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 ]

joanusdmentia

Jul 8, 2006 at 4:45 AM

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.

MaulingMonkey

Jul 8, 2006 at 5:14 AM

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.

localrob2

Jul 8, 2006 at 11:00 AM

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?

Alpha_ProgDes

Jul 8, 2006 at 11:36 AM

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.

daerid

Jul 8, 2006 at 12:15 PM

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.

CTar

Jul 8, 2006 at 12:16 PM

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.

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  }}

localrob2

Jul 8, 2006 at 12:35 PM

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.

jflanglois

Jul 8, 2006 at 1:54 PM

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.

localrob2

Jul 8, 2006 at 2:25 PM

I will definitly look into boost. Thanks.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.