Back to For Beginners

std::map is driving me insane!

For Beginners

Started by swinchen August 24, 2005 10:21 AM

30 comments, last by swinchen 18 years, 7 months ago

swinchen

224

Author

August 24, 2005 10:21 AM

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

CoffeeMug

852

August 24, 2005 10:29 AM

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]

defmacro.org

Wuntvor

170

August 24, 2005 10:30 AM

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. :)

Squirm

481

August 24, 2005 10:33 AM

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).

swinchen

224

Author

August 24, 2005 10:37 AM

Quote:Original post by CoffeeMug
There 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...

Seriema

634

August 24, 2005 10:37 AM

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 =(

[ ThumbView: Adds thumbnail support for DDS, PCX, TGA and 16 other imagetypes for Windows XP Explorer. ] [ Chocolate peanuts: Brazilian recipe for home made chocolate covered peanuts. Pure coding pleasure. ]

scott_l_smith

162

August 24, 2005 10:42 AM

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

CoffeeMug

852

August 24, 2005 10:45 AM

Quote:Original post by swinchen
I 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.

defmacro.org

pammatt

211

August 24, 2005 10:57 AM

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.

CoffeeMug

852

August 24, 2005 11:12 AM

Quote:Original post by pammatt
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.

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 pammatt
As 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 pammatt
As 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).

defmacro.org

std::map is driving me insane!

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

std::map is driving me insane!

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines