Exercise for the day:

posted in Author, Programmer, Bag of Wind(TM)

Published March 23, 2005

The following code is poorly written. How /SHOULD/ it be written? A big plate of virtual crawdads for the first person to get it right.

void ResourceManager::DeleteMap(ResMap &theMap){	for (ResMap::iterator anItr = theMap.begin(); anItr != theMap.end(); ++anItr)		delete anItr->second;	theMap.clear();}

To help out: ResMap is a typedef for map, although this is readily apparent from the code.

Previous Entry New Snake binaries

Next Entry Lessons Learned

0 likes 18 comments

Comments

noaktree

void ResourceManager::DeleteMap(ResMap &theMap)
{
theMap.erase(theMap.begin(), theMap.end());
}

March 23, 2005 10:10 AM

VisualLR

Quote:
void ResourceManager::DeleteMap(ResMap &theMap)
{
theMap.erase(theMap.begin(), theMap.end());
}

That won't call delete for the BaseRes*


void ResourceManager::DeleteMap(ResMap &theMap)
{
    for (ResMap::iterator anItr = theMap.begin(); anItr != theMap.end(); ++anItr)
    {
        BaseRes* pBaseRes = static_cast<BaseRes*>( anItr->second );
        if ( pBaseRes )
             delete pBaseRes;

    }

    theMap.clear();
}

March 23, 2005 10:17 AM

TANSTAAFL

VisualLR is on the right track, but he's not quite there yet.

and noak, your solution would cause a memory leak.

March 23, 2005 10:35 AM

Rob Loach


void ResourceManager::DeleteMap(ResMap &theMap)
{
	while(!theMap.empty()){
		delete theMap.end()->second;
		theMap.erase(theMap.end());
	}
}

...I haven't tried it.

March 23, 2005 10:38 AM

noaktree

If it's allowed ..the resource could be a smart pointer. This would ensure automatic deletion of the resource using the code I posted above. Whaja thank?

March 23, 2005 11:15 AM

TANSTAAFL

And Rob has an access violation, because end() doesn't return an iterator with any data attached to it.

Thus far, the best I can come up with:


template <class value_t> void SafeDelete2nd(value_t& theValue)
{
	if(theValue.second)
	{
		delete theValue.second;
		theValue.second=0;
	}
}

void ResourceManager::DeleteMap(ResMap &theMap)
{
	for_each(theMap.begin(),theMap.end(),SafeDelete2nd<ResMap::value_type>);
	theMap.clear();
}

March 23, 2005 11:27 AM

TANSTAAFL

Quote:If it's allowed ..the resource could be a smart pointer. This would ensure automatic deletion of the resource using the code I posted above. Whaja thank?

Unfortunately, this is not a code design problem... this is a code maintenance problem. The code I originally posted actually exists, so going back and making the resources smart pointers would be a lot more work than simply checking for null when going through the list.

March 23, 2005 11:30 AM

Jesse Chounard

Do you have delete overloaded for type BaseRes?

Otherwise, there is no reason to verify that your pointer doesn't point to null, because that safety net is built into the standard C++ delete.

I guess it's an issue if you're using a compiler that doesn't follow that rule.

March 23, 2005 01:27 PM

TANSTAAFL

Actually, Jesse brings up a good point.

So, my "best" code for this gets modified:


template <class value_t> void SafeDelete2nd(value_t& theValue)
{
	delete theValue.second;
	theValue.second=0;
}

void ResourceManager::DeleteMap(ResMap &theMap)
{
	for_each(theMap.begin(),theMap.end(),SafeDelete2nd<ResMap::value_type>);
	theMap.clear();
}

I still do want something set to 0 after a deletion, but if it happens to be 0 already, go ahead, as delete doesn't do anything.

Mainly, my concern with the original code was the hand-rolled loop with the iterator instead of for_each.

March 23, 2005 02:45 PM

Jesse Chounard

Is that really going to gain you anything over this? I've never used for_each, and this is almost exactly what some of my code looks like, so I'm really curious.

ResMap::iterator anIter = theMap.begin();
while(anIter != theMap.end())
{
    delete anIter->second;
    ++anIter;
}
theMap.clear();

While it's good form, you don't need to null-terminate the pointer in this case, since you make it irrelevant it by clearing the map anyway. (I've never used the source tags before, forgive me if this doesn't come through correctly.)

March 23, 2005 03:17 PM

TANSTAAFL

The reason I set the pointer to 0 in my template function is because I cannot guarantee that I will only be using that function when I'm about to clear the map.

The reason for using for_each is based on Item 43(pg 181) of Effective STL, by Scott Meyers, titled: "Prefer algorithm calls to hand-written loops."

He goes on for 9 pages of "why", which I won't repeat nor even summarize here, except to state that "he's right".

March 23, 2005 03:56 PM

TANSTAAFL

Actually, I'll quote his three reasons:

Quote:
Effective STL by Scott Meyers, Item 43, Page 182
Efficiency: Algorithms are often more efficient than the loops programmers produce.

Correctness: Writing loops is more subject to errors than is calling algorithms.

Maintainability: Algorithm calls often yield code that is clearer and more straightforward than the corresponding explicit loops.

March 23, 2005 04:01 PM

Jesse Chounard

More Effective C++ and Effective STL have been on my wish list for awhile. (I have Effective C++.) I think I'll finally get off my butt and order them.

And thanks for the page number, I'll look into it asap.

March 23, 2005 04:02 PM

VisualLR

I have those books, I guess it's time for a re-read =)

I had forgotten about for_each, I've only used it when using functors, so it didn't immediately come to mind for this.

March 23, 2005 04:09 PM

johnhattan

STL makes head hurty hurty.

FIRE BAD!!!

March 23, 2005 10:40 PM

VisualLR

So.. do I get at least half a plate of virtual crawdads for almost getting it right?

March 23, 2005 11:07 PM

TANSTAAFL

Everybody who played can have as many virtual crawdads as they can eat.

March 24, 2005 07:27 AM

You must log in to join the conversation.

Don't have a GameDev.net account? Sign up!

TANSTAAFL

Author

Exercise for the day:

Comments

TANSTAAFL

Latest Entries

Music To My Ears

Getting There...

Guess Chess

iPhone JetLag

iPhone JetLag

JetLag on the iPhone

JetLag on the iPhone

JetLag on the iPhone

A New Era Has Dawned

Need Playtesters for HamQuest

Exercise for the day:

Comments

TANSTAAFL

Latest Entries

Music To My Ears

Getting There...

Guess Chess

iPhone JetLag

iPhone JetLag

JetLag on the iPhone

JetLag on the iPhone

JetLag on the iPhone

A New Era Has Dawned

Need Playtesters for HamQuest

Reticulating splines