Take the 2-minute tour ×
Game Development Stack Exchange is a question and answer site for professional and independent game developers. It's 100% free, no registration required.

I've been trying to make a pretty simple space shooter game with C++, and I have been having a lot of trouble trying to use lists to store enemies and bullets.

Basically I followed the post here almost exactly to store my bullets:

SDL Bullet Movement

I've done something similar to store my enemies.

However, when I call bullets.erase(it++), for some reason the bullet is not erased.

When the bullet movement is run for the next frame, it tries to re delete the bullet and segfaults the program. When I comment out the erase line, it runs fine, but the bullets are then never erased from the list...

Is there any reason why the elements of the list aren't being deleted? I also set it up to print the number of elements in the list for every iteration, and it does not go down after deleting.

Thanks!

EDIT:

Here's the specific code I'm using to store my enemies and having them act:

std::list<Grunt*> doGrunts(std::list<Grunt*> grunts)
{
  for(std::list<Grunt*>::iterator it = grunts.begin(); it != grunts.end();)
    {

      if((*it)->getHull() == 0)
        {                                       
          delete * it;                                             
          grunts.erase(it++);
        }
      else
        {

          (**it).doUnit(grunts, it);
          ++it;
        }
     }
  }

Grunt is my enemy class, and the list grunts is a global variable. Is that my problem? When I pass the global into the function it becomes local? I assumed lists would be a reference type so thought this wouldn't be a problem.

Sorry if this was a dumb question, I'm very new to C++, this is the first major thing I'm working on.

share|improve this question
    
Can you post the code/function/algorithm that you are using to iterate over, decide which need removing from the list, and attempted deletion from said list? –  Casey Jun 29 '12 at 2:29
    
I don't see any return... –  Klaim Jun 30 '12 at 6:06

5 Answers 5

up vote 2 down vote accepted

container.erase calls the destructor of the contained object if it has one. pointers like Grunt* do not have destructors. You must manually delete the pointer THEN call erase.

You also have to be careful of invalidation of iterators, incrementing an invalidated pointer will cause a BLOCK_HEAD failure.

pseudo-code:

for each iterator "it" in container //(DO NOT increment iterator in for loop block statement!)
    if it->position is off_screen {
        delete *it;
        *it = NULL; //Prevents seg fault due to double-delete.
        container.erase(it++);
    } else {
        ++it;
    }

SUPER EDIT

In light of the code you posted:

1) Yes, a copy of grunts is created because the list is being passed by value. It is also being shadowed because you named the local variable the same as the global one. 2) Why are you passing a GLOBAL variable around? Either A) just use it, or B) make it non-global and pass by reference.

share|improve this answer
    
Hi Casey, thanks for your response. I did this, but doing this would cause seg faults because the container would seemingly reset and try to delete the already deleted object... –  XD_dued Jun 29 '12 at 2:20
    
@XD_dued: I was in the middle of editing the answer while you were commenting, see my changes. –  Casey Jun 29 '12 at 2:24
    
Alright thanks a lot for your time... Man now I feel really dumb :). –  XD_dued Jun 29 '12 at 2:47
    
There is no such thing as operator~ on a pointer. The destructor is not an operator. –  DeadMG Jun 29 '12 at 4:11
1  
And there is such a thing as operator~, but it's something completely different: bitwise negation. –  MaulingMonkey Jun 29 '12 at 4:30

You need to use resource-managing classes. Deleting your own memory is extremely error-prone. The only viable solution in the long term is to use classes like std::shared_ptr, std::unique_ptr, or simply storing your bullets by value. Why on earth would you use a linked list of pointers? That's quite unnecessary.

share|improve this answer
    
I can think of a very good reason: LSP. –  Casey Jun 29 '12 at 5:28
    
When I worked with C++ pretty much the only thing I stored in lists and vectors were pointers - both in personal projects and in the games industry. Many items can't be effectively stored by value and even where they can, it's typically very inefficient. –  Kylotan Jun 29 '12 at 9:32

If it is an iterator for the std::list<Bullet*> bullets, calling bullets.erase(*it) should be invalid since *it is a Bullet* not an std::list<Bullet*>::iterator and erase requires an iterator. If you want to remove elements by referring to their value you could try the remove function.

EDIT: Also, with an std::list you do not have to worry about invalidating iterators using list::erase. I am not sure if the same guarantee is given for list::remove.

share|improve this answer
    
Sorry I made a mistake in the post. I did to bullets.erase(it++). –  XD_dued Jun 29 '12 at 2:25
    
Are you sure that statement is being executed? Is your code exactly the same as in the linked question? –  Chewy Gumball Jun 29 '12 at 2:30

There are two things that I can see in that code that could cause problems.

First, you aren't checking for NULL Grunt pointers. You dereference (*it)->GetHull, and if (*it) in this case is NULL you'll get your segfault.

Second, when you call erase, you could be moving memory about in the list. It's not really safe to use an iterator (i.e. a pointer into the list structure) after the list is modified. But specifically in this case, you should NOT be incrementing it, as that will skip an element. Like this

(G1)->(G2)->it->end -> erase (it) -> (G1)->(G2)->[it]end (maybe) -> it++ -> ??? the iterator's already at the end!

I can't say for sure if that's happening, but you shouldn't do it anyway, as you'll skip over elements of the list.

share|improve this answer

For the record, the std::list::erase() method returns the next iterator in the list, making incrementing a counter redundant, and in my opinion, a safer method for erasing container elements.

Assuming grunts is a std::list<Grunt*>:

auto iter = grunts.begin();
while(iter != grunts.end())
{
  delete *iter; // No need to set NULL here, if you're going to remove from list.
  iter = grunts.erase(iter);
}

Furthermore, you should start collecting a nice set of helper macros for stuff like deleting pointers. This will save you some headaches later.

#define SAFE_DELETE(x) if(x){delete x; x=NULL;} // Keep in a common header somewhere..
Grunt* pGrunt = new Grunt();
Grunt* pBadGrunt = NULL;
SAFE_DELETE(pGrunt); // pGrunt is now free'd and NULL
SAFE_DELETE(pBadGrunt); // Nothing happens, pointer is still NULL
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.