Program crashes with STD::map find()

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

Recommended Posts

Hello, I have a function that crashes my program when it tries to find an object in a map. Here is the code. Is it possible to see why this might be happening because I am stuck on this one...
void CollisionSet::addCollision( GameComponent* first, GameComponent* second )
{
// adds the collision to the first component's list
std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );

/*if ( firstComponent == this->_collisionMap->end() )
{
CollisionQueue* firstQueue;
this->_collisionMap->insert( std::make_pair( first, firstQueue ) );
}

Everything in the function is commented out, so this has to be the problem. Thanks

Share on other sites
Check your pointer to make sure it is pointing to a valid object. Thats my first guess. (The _collisionMap pointer)

Share on other sites
Well I'll add something to check it directly, but they should be valid since I do some things with them before calling that function. I just realized that it's almost impossible for anyone to determine the problem from what I posted, so I'll look over it some more. Thanks anyways.

Share on other sites
I'm talking about something like the following...You never actually create a map object to point to, so it crashes.

#include <map>int main(){	std::map<int *, char *> * pMap;	std::map< int*, char* >::iterator first = pMap->find( NULL );	return 0;}

needs to be:

#include <map>int main(){	std::map<int *, char *> * pMap = new std::map<int*,char*>;	std::map< int*, char* >::iterator first = pMap->find( NULL );	delete pMap;	return 0;}

It might also be that your object has been destructed and your this pointer is garbage.

Share on other sites
Are you sure you are wanting to insert a non-assigned pointer into your _collisionMap.

CollisionQueue* firstQueue; // this is not pointing to a valid object. are you handling this?
this->_collisionMap->insert( std::make_pair( first, firstQueue ) );

Share on other sites
Ah right, I did forget to add that. However, it still crashes. What a pain in the ass! :P

Share on other sites
Add the rest of your code so we can look at more of it... Please point out which line it is crashing on too. Thanks.

Share on other sites
Quote:
 Original post by RuntCreatureCheck your pointer to make sure it is pointing to a valid object. Thats my first guess. (The _collisionMap pointer)

If "find()" isn't crashing, and either "end()" or "insert()" is where the crash happens, it's likely that the pointer points to "something", but that the vtable is messed up.

It would help to see how the map instance is created... as you suggest.

Share on other sites
Ok here is the constructor:

CollisionSet::CollisionSet(){    // initialize the collision set    this->_collisionMap = new std::map< GameComponent*, CollisionQueue* >();}

and here is the whole function. I commented out everything then started with the first line which happened to crash the game. So that's how I figured that it is the problem.

void CollisionSet::addCollision( GameComponent* first, GameComponent* second ){    // adds the collision to the first component's list    std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );    if ( firstComponent == this->_collisionMap->end() )    {        CollisionQueue* firstQueue;        this->_collisionMap->insert( std::make_pair( first, firstQueue ) );    }        Collision firstCollision( second );    firstComponent->second->addCollision( &firstCollision );    // adds the collision to the second component's list    std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = this->_collisionMap->find( second );    if ( secondComponent == this->_collisionMap->end() )    {        CollisionQueue* secondQueue;        this->_collisionMap->insert( std::make_pair( second, secondQueue ) );    }        Collision secondCollision( first );    secondComponent->second->addCollision( &secondCollision ); }

I'll have to fix that make_pair too. A buddy of mine wrote this code, so I'm just trying to fix his bugs.

Share on other sites
I noticed that

Won't work if it is pointing to _collisionMap->end(), so I made sure that firstComponent points to the new element when the new pair is made. Also, after fixing that, it will do the initial search for first without crashing. I'm going to test it to see if it's working properly now.

Share on other sites
Why are you using pointers to GameComponents as your keys? Where are the pointed-at GameComponents? Similarly about the values.

Share on other sites
Quote:
 Original post by TokikoI noticed that firstComponent->second->addCollision( firstCollision ); Won't work if it is pointing to _collisionMap->end(), so I made sure that firstComponent points to the new element when the new pair is made. Also, after fixing that, it will do the initial search for first without crashing. I'm going to test it to see if it's working properly now.

secondComponent->second->addCollision( &secondCollision );

And I'm with Zahlman... why are you inserting "pointers to nothing" into your map? You may as well not insert anything... and just reference your map like this:

(*_collisionMap)[first] = new CollisionQueue();

Your calls to "insert" aren't having any affect, as written. If you invoke the bracket ([]) operator, you can just make an assignment whenever you want. If the key is in the map already... great! You'll wipe out whatever was there before.

(Not a good idea if it was a real pointer to something)

But as written, "insert" gives you absolutely nothing.

I would suggest using reference counted smart pointers instead of raw pointers in your map to make this strategy more air-tight.

Share on other sites
Thanks Verg, I'll have to try that out after work. I should have noticed these problems, but like I said, I didn't look over it in too much detail as I didn't write it.

I was thinking of using smart pointers in the game. I've already run into problems deleting pointers and causing memory violations, so I obviously don't have enough experience with them yet. I'll have to look into that.

Share on other sites
Here's what I have so far except I'm still having difficulties. Everything will work if the last line is commented out, this one: (*this->_collisionMap)[ second ]->addCollision( new Collision( first ) );
It is adding a collision to the second game components collision queue.
Someone asked above why we use pointers for the maps key. We have to because we are passing in derived objects of that GameComponent type.

void CollisionSet::addCollision( GameComponent* first, GameComponent* second ){    // adds the collision to the first component's list    std::map< GameComponent*, CollisionQueue* >::iterator firstComponent = this->_collisionMap->find( first );    if ( firstComponent == this->_collisionMap->end() )    {         // this component does not yet have a queue for collisions, we must put a new one in the map         this->_collisionMap->insert( std::make_pair( first, new CollisionQueue() ) );    }    // create a collision and add it to the first components collision queue    (*this->_collisionMap)[ first ]->addCollision( new Collision( first ) );    // adds the collision to the second component's list    std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = this->_collisionMap->find( second );    if ( secondComponent == this->_collisionMap->end() )    {         // this component does not yet have a queue for collisions, we must put a new one in the map         this->_collisionMap->insert( std::make_pair( second, new CollisionQueue() ) );    }    // create a collision and add it to the second components collision queue    // comment this line out for the game to run - leave it and the game crashes on a collision    (*this->_collisionMap)[ second ]->addCollision( new Collision( first ) ); }

So does anyone have any ideas why this only crashes for the 2nd part of the code? Could it be some weird memory problem with all these pointers? I'm at a loss.

Share on other sites
Sorry everyone, my friend is to blame for all of these errors. He first forgot to allocate the map in the constructor (which I fixed) but we just noticed that he didn't allocate the list inside the collisionQueue. All is good now. Thanks for everyone's help.

Share on other sites
Let's see if we can clean this up:

void CollisionSet::addCollision( GameComponent* first, GameComponent* second ){    std::map< GameComponent*, CollisionQueue* >::iterator firstComponent =  _collisionMap->find( first );    if ( firstComponent == _collisionMap->end() )         (*_collisionMap)[first] = new CollisionQueue();    (*_collisionMap)[first]->addCollision( new Collision( first ) );    std::map< GameComponent*, CollisionQueue* >::iterator secondComponent = _collisionMap->find( second );    if ( secondComponent == _collisionMap->end() )         (*_collisionMap)[second] = new CollisionQueue();    (*_collisionMap)[ second ]->addCollision( new Collision( first ) ); }

Are you SURE you want that last line to read like that, and not like this??

    (*_collisionMap)[ second ]->addCollision( new Collision( second ) );

Also... take out the spurious "this->" stuff... unless you're passing in another parameter with the same name (_collisionMap) or you have one in the namespace surrounding your class (global to it)... you don't need it, and it makes everything easier to read.

And there's some obvious duplication there... you could just call one function twice and pass in the parameters.

C

Share on other sites
Quote:
 Are you SURE you want that last line to read like that, and not like this?? (*_collisionMap)[ second ]->addCollision( new Collision( second ) );

Sorry there was a typo with the first collision and it should be passing second. So if I didn't type that wrong, you would be correct :P
Quote:
 Also... take out the spurious "this->" stuff... unless you're passing in another parameter with the same name (_collisionMap) or you have one in the namespace surrounding your class (global to it)... you don't need it, and it makes everything easier to read.

Yes that would be easier to read, but the only reason why we use it for member variables is because we developed that habit at school. Our CS teachers wanted us to use it in our assignments and it may have been counted as a mark? Can't remember.
(*_collisionMap)[first] = new CollisionQueue();
Now with this line here. I was going to use that earlier, but I figured that it wouldn't work since you are trying to find the value paired with first, but if that key hasn't been added, how can you make a new collision? So I guess it will insert a new element if it is not found?

Thanks for the help. We went from a large broken function, to a much smaller and easier to read function :)

Share on other sites
Quote:
 Original post by TokikoSorry everyone, my friend is to blame for all of these errors. He first forgot to allocate the map in the constructor (which I fixed) but we just noticed that he didn't allocate the list inside the collisionQueue. All is good now. Thanks for everyone's help.

Why would you be allocating instances of standard library containers? Just hold them as members.

The whole reason you are using standard library containers is so that they can do memory management work for you. Why go on and do it anyway?

Share on other sites
Quote:
 Original post by ZahlmanWhy would you be allocating instances of standard library containers? Just hold them as members.The whole reason you are using standard library containers is so that they can do memory management work for you. Why go on and do it anyway?

I must have worded that wrong. I didn't create the object in the constructor. Like so:
map<int, string>* obj = new map<int, string>();

Share on other sites
Quote:
 Original post by TokikoI must have worded that wrong. I didn't create the object in the constructor. Like so: map* obj = new map();
That's a little confusing. If the above is an excerpt from your actual code, then you are in fact allocating the map dynamically in the constructor. Which then brings us back to Zahlman's question: why?

[Edit: Actually, I'm guessing (based on the use of a local temporary) that the above code excerpt was just an example, but the samples you posted earlier indicate that you are indeed allocating the container dynamically in the object's constructor.]

Share on other sites
Quote:
Original post by Tokiko
Quote:
 Are you SURE you want that last line to read like that, and not like this?? (*_collisionMap)[ second ]->addCollision( new Collision( second ) );

Sorry there was a typo with the first collision and it should be passing second. So if I didn't type that wrong, you would be correct :P

Well then you can simplify the code even more...

void CollisionSet::addCollision( GameComponent* component ){    if ( _collisionMap->find( component ) == _collisionMap->end() )         (*_collisionMap)[component] = new CollisionQueue();    (*_collisionMap)[component]->addCollision( new Collision( component ) );}// And you would just call it twice, once for each component... // since they don't depend on one another in the original functionaddCollision( first );addCollision( second );

Quote:
 (*_collisionMap)[first] = new CollisionQueue();Now with this line here. I was going to use that earlier, but I figured that it wouldn't work since you are trying to find the value paired with first, but if that key hasn't been added, how can you make a new collision? So I guess it will insert a new element if it is not found?

Exactly. But you should check for the element first or you would erase the old CollisionQueue, if it already existed.

And I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.

Share on other sites
Quote:
 Original post by VergAnd I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.

This has been brought up a couple times, but I'm still not sure how I can fix this. I thought if you have a map declared in the class as a member, there are no problems with initializing it in the constructor. Can you or someone give me an example of what you would do in this case?

Quote:
 void CollisionSet::addCollision( GameComponent* component ){ if ( _collisionMap->find( component ) == _collisionMap->end() ) (*_collisionMap)[component] = new CollisionQueue(); (*_collisionMap)[component]->addCollision( new Collision( component ) );}// And you would just call it twice, once for each component... // since they don't depend on one another in the original functionaddCollision( first );addCollision( second );

I wouldn't be able to simplify it to that. When I make the collision object for the first component, I need to pass the second component so that I know what collided with it. Same goes for the second component - I pass the first one to the collision object. With your code, you would be creating a collision between the component and itself.

Share on other sites
Quote:
Original post by Tokiko
Quote:
 Original post by VergAnd I'm with Zahlman... is there a compelling reason to dynamically create a map? You could just use the map as an instance variable, and then you don't have to destroy it later.

This has been brought up a couple times, but I'm still not sure how I can fix this. I thought if you have a map declared in the class as a member, there are no problems with initializing it in the constructor. Can you or someone give me an example of what you would do in this case?

I believe they meant like so:
class YourClass {    std::map<GameComponent*, CollisionQueue*> _collisionMap;    [...]};

The _collisionMap will be allocated on the stack, not the free-store (heap). Thus, no messy allocation/destruction (new/delete).

Share on other sites
Ok, so in other words, don't make it a pointer? Because that is the only difference between what I have and what you suggested.

Share on other sites
Quote:
 Original post by TokikoOk, so in other words, don't make it a pointer?
Right - just make it a member variable of the class (as agi_shi's example illustrates).