4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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? \$\endgroup\$
    – Casey
    Jun 29, 2012 at 2:29
  • \$\begingroup\$ I don't see any return... \$\endgroup\$
    – Klaim
    Jun 30, 2012 at 6:06

5 Answers 5

2
\$\begingroup\$

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.

\$\endgroup\$
8
  • \$\begingroup\$ 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... \$\endgroup\$
    – XD_dued
    Jun 29, 2012 at 2:20
  • \$\begingroup\$ @XD_dued: I was in the middle of editing the answer while you were commenting, see my changes. \$\endgroup\$
    – Casey
    Jun 29, 2012 at 2:24
  • \$\begingroup\$ Alright thanks a lot for your time... Man now I feel really dumb :). \$\endgroup\$
    – XD_dued
    Jun 29, 2012 at 2:47
  • \$\begingroup\$ There is no such thing as operator~ on a pointer. The destructor is not an operator. \$\endgroup\$
    – DeadMG
    Jun 29, 2012 at 4:11
  • 1
    \$\begingroup\$ And there is such a thing as operator~, but it's something completely different: bitwise negation. \$\endgroup\$ Jun 29, 2012 at 4:30
2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I can think of a very good reason: LSP. \$\endgroup\$
    – Casey
    Jun 29, 2012 at 5:28
  • \$\begingroup\$ 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. \$\endgroup\$
    – Kylotan
    Jun 29, 2012 at 9:32
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Sorry I made a mistake in the post. I did to bullets.erase(it++). \$\endgroup\$
    – XD_dued
    Jun 29, 2012 at 2:25
  • \$\begingroup\$ Are you sure that statement is being executed? Is your code exactly the same as in the linked question? \$\endgroup\$ Jun 29, 2012 at 2:30
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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
\$\endgroup\$

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .