Public Group

# std::map is driving me insane!

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

## Recommended Posts

Ok, lets see if I can word this...
typedef std::map<std::string, CTexture*> t_textureMap;
typedef t_textureMap::iterator t_textureItor;
typedef std::pair<std::string, CTexture*> t_texturePair;


So I have a map m_textures (type t_textureMap) that I am trying to itterate through and delete all CTextures that are stored in the map, and then erase the map entry itself. The source block below does NOT work, but if I replace the last two lines with:
m_textures.erase(textureItor++);  // WORKS!


it works fine. The version that doesn't work causes the application to crash. Shouldn't this be the exact same thing? It is making me want to kick the computer.
CTextureMgr::~CTextureMgr()
{
CTexture* texPtr = NULL;
t_textureItor textureItor = m_textures.begin();
while (textureItor != m_textures.end())
{
texPtr = textureItor->second;

printf("~CTextureMgr: %s\n", texPtr->filename.c_str());
fflush(NULL);

glDeleteTextures(1,&(texPtr->texID));
delete texPtr;
texPtr = NULL;
m_textures.erase(textureItor);  // DOES NOT WORK! :(
textureItor++;
}
}



##### Share on other sites
In the crash case:
1. Delete the iterator
2. Try to increment the iterator

In the good case:
1. Push the iterator value on the stack
2. Increment the iterator
3. Use the stack value to erase the iterator

As you can see, in the first case you delete the iterator first, then try to increment. In the second case you first save the current value, then increment, then delete the current value.

In any case, you should store the textures in the map using smart pointers and put deleting of gl stuff in the destructor of CTexture. Also, learn to use for_each construct and its variants. There is at least one thousand and one reason why the code you have now is terrible [grin]

##### Share on other sites
Hm, I am not sure if you should be modifying the map at all while iterating through it.. I am actually surprised that it works if you exchange those lines (all I know is you can't modify a collection in C# while iterating through it, which is kind of the same thing). After all, if you erase the current item, what is the iterator pointing to?

Personally, I'd just iterate through it, clear up the resources and then clear the map afterwards.

EDIT: Yeah, CoffeeMug put it better than me. :)

##### Share on other sites
m_textures.erase(textureItor); will invalidate the iterator, and return you a new iterator which is valid:

textureItor = m_textures.erase(textureItor);

However, I can't actually remember which iterator it returns (the next one, I think, which makes the loop awkward).

edit: previous posters smart pointers = good approach. However, previous posters "store copy of iterator" isn't safe, I don't think (it might be - I'm no expert on the standard - it certainly wouldn't be safe if data was being added because there's no guarantee that the copy is still valid after the container has been grown, and hence possibly moved, but erase might have been defined to make this legal). (Put another way, your ++ variant is working, but only by chance, and may fail with other compilers/platforms/memory/blah blah blah).

##### Share on other sites
Quote:
 Original post by CoffeeMugThere is at least one thousand and one reason why the code you have now is terrible [grin]

Wow, is it really that bad? I have a reference count in CTexture (a struct). Seeing this where the texture manager is getting destoryed I figured it would be a pretty good place to destroy the textures.

I didnt think it was that bad...

##### Share on other sites
Why you get an error is already explained so I'll skip that.

erase() returns a valid iterator to the next element. I'm pretty sure all containers do this. So in short, catch what erase returns like so:

iter = mymap.erase( iter );

A good bok for learning these quirks is Effective STL by Scott Meyers.

EDIT: I write too slow =(

##### Share on other sites
Although only mildly relevant, it is good practice (read: do it) to pre-increment your iterators in C++. The post increment operator requires a copy of the object to be created, which is always a bad thing. If the underlying representation of the iterator (or any other class that overloads the post_increment) is large, there is a lot of copying going on! Of course this extends beyond iterators, but they are of particular importance beacuse they are so frequently incremented. Just an observation. Good luck.

Scott

##### Share on other sites
Quote:
 Original post by swinchenI didnt think it was that bad...

It's not that bad, but you're effectively using C style coding in between C++ architecture. You shouldn't do that unless you can put the reasons for doing so in words. Why did you write your own reference counting mechanism instead of using boost::shared_ptr? Why do you use a custom iteration loop instead of using for_each? Why do you reduce encapsulation by putting cleanup code for CTexture outside of its destructor?

There are obviously a million ways to do the same thing. You should familiarize yourself with alternatives and try to use C++ friendly code unless you can explicitly state why you wouldn't want to do that in your particular situation.

##### Share on other sites
Why aren't you using boost::shared_ptr?? Well, many people do not want a hugely bloated library when they merely want referenced counted pointers. As a seasoned software engineer, I dont' even want to use boost smart pointers.

As for using for_each, sure its cleaner looking, but that's no reason to say that writing a normal for loop is bad. And I would go easy with accusing people of using C style coding in C++. Keep in mind, the enourmous amount of "hacks" in C++ to acheive good object oriented design.

##### Share on other sites
Quote:
 Original post by pammattWhy aren't you using boost::shared_ptr?? Well, many people do not want a hugely bloated library when they merely want referenced counted pointers.

Boost's smart pointers are implemented fully in the header files and require no linking. They generate safer, more compact and more efficient code than we can with a home baked solution. For things that do require linking, boost allows you to generate small libraries only for features that you need. Even if you did link to the full library, the linker would take only the necessary components.
Quote:
 Original post by pammattAs a seasoned software engineer, I dont' even want to use boost smart pointers.

That's fine, if you could give a good reason for not using them. If you can't, then you should probably use them.
Quote:
 Original post by pammattAs for using for_each, sure its cleaner looking, but that's no reason to say that writing a normal for loop is bad.

I didn't say writing a normal for loop is bad. I said that if you iterate through an STL collection you should probably use for_each unless you can give a good reason for not doing so. You will end up with a safer, more efficient, less buggy code by using for_each. The original poster clearly didn't know how to write a proper for loop to iterate through a collection (which is fine, I have to look this up all the time). Using for_each would have saved a lot of trouble (though in this case smart pointers allong with collection.clear() is a better idea).

• ### What is your GameDev Story?

In 2019 we are celebrating 20 years of GameDev.net! Share your GameDev Story with us.

• 31
• 16
• 11
• 10
• 11
• ### Forum Statistics

• Total Topics
634114
• Total Posts
3015593
×