Sign in to follow this  
Maxamor

[C++][STL][SDL] Memory Management (std::string)

Recommended Posts

Maxamor    361
Hey all. I have experience with Java and C# but not as much with C++. I have written a small class to manage my texture for a little SDL app I am writing. The class is your average texture cache. You "request" a texture by handing it a std::string, and you get back a pointer to an SDL_Surface. See the listing below for the header/source. As far as I know this class is working as expected--but I'm not sure I'm cleaning up everything that I could be. I associate "std::string" with "SDL_Surface*" where the ""std::string" is the key, and the "SDL_Surface" is the value. When I want to destroy the std::map that these string-surface associations is stored in, I iteratr from the beginning to the end and call SDL_FreeSurface, and then delete on the SDL_Surface pointers. After freeing the surfaces and deleting all of the pointers, I call std::map.clear(); My question is, will this destroy the strings and clean up the memory they occupied, or will their memory be a leak?
// TextureCache.hpp
#ifndef __TEXTURECACHE__
#define __TEXTURECACHE__

// Includes go here.
#include <map>
#include <string>
#include <SDL/SDL.h>
#include <SDL/SDL_image.h>

using namespace std;

typedef pair<string, SDL_Surface*> StringTexturePair;

class TextureCache {
    private:
        std::map<string, SDL_Surface*> textureMap;
        std::map<string, SDL_Surface*>::iterator textureIterator;
        bool Contains(string);
    protected:
    public:
        TextureCache();
        ~TextureCache();
        void Clear();
        SDL_Surface* Get(string);
};

#endif
// TextureCache.cpp
#include "../../header/graphics/TextureCache.hpp"

TextureCache::TextureCache()
{
    SDL_Surface* pointer = Get("maxamor.gif"); // For testing purposes.
    fprintf(stdout, "TextureCache is working properly. %i\n", pointer->w);
}

TextureCache::~TextureCache()
{
    Clear();
}

void TextureCache::Clear()
{
    if(textureMap.size() < 1)
        return;

    for(textureIterator = textureMap.begin(); textureIterator != textureMap.end(); textureIterator++)
    {
        fprintf(stdout, "Deleting %s\n", textureIterator->first.c_str());
        SDL_FreeSurface(textureIterator->second);
        delete textureIterator->second;
    }

    textureMap.clear();
}

bool TextureCache::Contains(string textureName)
{
    textureIterator = textureMap.find(textureName);
    return (textureIterator != textureMap.end());
}

SDL_Surface* TextureCache::Get(string textureName)
{
    // Texture needs to be loaded.
    if(!Contains(textureName)) {
        SDL_Surface* texturePointer = IMG_Load(textureName.c_str());
        textureMap.insert(StringTexturePair(textureName, texturePointer));
    }

    // Texture has already been loaded.
    return textureMap.find(textureName)->second;
}

// Newline to follow this comment.

Share this post


Link to post
Share on other sites
Agony    3452
std::string and std::map will clean up after themselves, even if you don't call .clear(), so you're good. Their destructors will do all the work necessary to clean up whatever resources they obtained on their own.

A few comments, however. Under most circumstances, you should pass std::string parameters by const reference (const std::string& Name), so that a copy doesn't need to be made, memory allocated, and so forth.

Also, it's generally better to use .empty() to determine if a container such as std::map is empty or not. .size() might take more than constant time to calculate, depending upon the implementation, but .empty(), as far as I'm aware, should always be a constant-time operation.

Lastly, it's generally recommended that you do not use the "using ***;" statement within a header file, but only in a source file. This is so that when another source file includes that header file, identifiers from other namespaces don't get unknowingly pulled into the global namespace. Since you're already using "std::" for std::map, go ahead and do the same for std::string, and then put "using namespace std;" near the top of your .cpp file, if you wish to save typing. (I always type "std::", but that's just a personal preference.)

Share this post


Link to post
Share on other sites
Julian90    736
ok just a couple of little things before i get to actually answering the question...

First, in header file you have the following line:
using namespace std
which is a really bad idea because and were that includes this header then has the entire std namespace "opened up" which will eventually result in name clashes in any large or moderate sized project :-).

Second, not really a big deal but i recomend using std::cout instead of fprintf() :-).

Now for your question :-), the strings will be freed automatically, you are storing a actual copy of the string rather then a pointer to some memory that contains a string :-).

More of a worry however is the following...

SDL_FreeSurface(textureIterator->second);
delete textureIterator->second;

now you said the following
Quote:
I iteratr from the beginning to the end and call SDL_FreeSurface, and then delete on the SDL_Surface pointers

I havnt used SDL but i would expect that SDL_FreeSurface() would free the memory pointed to by textureIterator->second meaning that you would be deleting memory that g=has been freed already :-). Although as i said i havnt used SDL so i could be wrong here.

Lastly just a semantic issue, but, you dont actually delete the pointer :-) you delete the memory it points to :-), Ok now i admit it im just being picky here lol ;).

Share this post


Link to post
Share on other sites
Maxamor    361
I appreciate your critique and am making the necessary changes.

Quote:
Under most circumstances, you should pass std::string parameters by const reference (const std::string& Name), so that a copy doesn't need to be made, memory allocated, and so forth.

Delightful! I have changed this as well.

Quote:
I havnt used SDL but i would expect that SDL_FreeSurface() would free the memory pointed to by textureIterator->second meaning that you would be deleting memory that g=has been freed already :-). Although as i said i havnt used SDL so i could be wrong here.

I agree. I'm getting the pointer from SDL, I might as well let SDL get rid of it--right? :)

Quote:
Lastly just a semantic issue, but, you dont actually delete the pointer :-) you delete the memory it points to :-)

Thanks for clarifying this.

As a side not, I'm not using to having to maintain Header files, but I am trying to keep them as tight and clean as possible. Thanks for giving me some tips.

Share this post


Link to post
Share on other sites
Zahlman    1682
You must always deallocate in a manner that is symmetrical to how you allocated.

How did you get the SDL_Surface* to point at a valid (and not previously-existing surface)? With 'new'? No? Then don't 'delete' it. You got it from an SDL library function, so you call the corresponding SDL library function to clean up (SDL_FreeSurface). Deallocating the memory is SDL's responsibility, because SDL allocated it. It could have allocated it in a variety of ways, and can't reasonably expect you to know how (it shouldn't try to do this through documentation!). In short, calling 'delete' on that pointer, whether or not you use SDL_FreeSurface as well, is just as bad as using delete[] on a single new'd object, or free() on a new[]'d array, or any form of delete on a malloc()'d allocation: it doesn't match. Technically speaking, undefined behaviour. Informally speaking, bang, you're dead.

Similarly, if you release code that could allocate memory, the deallocation is the responsibility of your code. Ideally you would make use of the RAII paradigm, so that the user doesn't have to do anything. Failing that (SDL "fails that" because it's really a C API), you provide cleanup functions. You *don't* tell the user "oh, you'll have to delete that pointer when you're done, because I used new when I gave it to you". That's irresponsible.

(And FFS, don't use fprintf(). We have much nicer I/O functionality now.)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this