Static class member destruction in C++

Question

I have a basic cache set up. Whenever a user requests a bitmap, it fetches or loads it from disk if it isn't already loaded, significantly reducing load times.

Currently, the design explicitly tells the user that they are responsible for freeing the individual bitmaps when they are done. That's fine, but the biggest problem I notice right away is that the cache is created on first use. There's no way to delete it without having the user expressly call a Release method (which seems like a bad idea). They are forced not only to release each individual bitmap, but also the cache even though they never created it.

Should I create Initialize and Release methods strictly to new and delete the std::map?

(It's named pool in the code because the member name hasn't been updated as of this posting.)

BitmapCache.h

#ifndef CBITMAPCACHE_H
#define CBITMAPCACHE_H

#include <allegro\file.h>
#include <allegro\gfx.h>
#include <allegro\draw.h>
#include <allegro\datafile.h>
#include <allegro\color.h>

#include <map>
#include <string>

struct BITMAP;

class BitmapCache {
public:

    static BITMAP* GetBitmap(std::string filename);
    static BITMAP* GetBitmap(BITMAP* file);
    static std::string GetBitmapFilename(BITMAP* file);
    static BITMAP* GetBlankBitmap(int width, int height);

protected:
private:

    static std::map<std::string, BITMAP*>* _pool;
    static void CleanCache();
};

#endif

BitmapCache.cpp

#include "CBitmapCache.h"

#include <algorithm>

std::map<std::string, BITMAP*>* BitmapCache::_pool = NULL;

BITMAP* BitmapCache::GetBitmap(std::string filename) {
    //Return NULL if a bad filename was passed.
    if(filename.empty()) return NULL;
    if(exists(filename.c_str()) == false) return NULL;

    //Reduce incorrect results by forcing slash equality.
    filename = fix_filename_slashes(&filename[0]);

    //Create pool on first use.
    if(_pool == NULL) _pool = new std::map<std::string, BITMAP*>();

    //Clean the pool if it's dirty.
    CleanCache();

    //Search for requested BITMAP.
    std::map<std::string, BITMAP*>::iterator _iter = _pool->find(filename);

    //If found, return it.
    if(_iter != _pool->end()) return _iter->second;

    //Otherwise, create it, store it, then return it.
    BITMAP* result = load_bmp(filename.c_str(), NULL);
    if(result == NULL) return NULL;
    _pool->insert(std::pair<std::string, BITMAP*>(filename, result));
    return result;
}

BITMAP* BitmapCache::GetBitmap(BITMAP* file) {
    if(file == NULL) return NULL;
    if(_pool == NULL) new std::map<std::string, BITMAP*>();

    CleanCache();

    for(std::map<std::string, BITMAP*>::iterator _iter = _pool->begin(); _iter != _pool->end(); ++_iter) {
        if(_iter->second != file) continue;
        return _iter->second;
    }
    return NULL;
}

std::string BitmapCache::GetBitmapFilename(BITMAP* file) {
    if(file == NULL) return std::string("");
    if(_pool == NULL) return std::string("");
    CleanCache();
    for(std::map<std::string, BITMAP*>::iterator _iter = _pool->begin(); _iter != _pool->end(); ++_iter) {
        if(_iter->second != file) continue;
        return _iter->first;
    }
    return std::string("");
}

BITMAP* BitmapCache::GetBlankBitmap(int width, int height) {

    //Smallest allowed size is 1x1.
    if(width < 1 || height < 1) return NULL;

    //Create pool on first use.
    if(_pool == NULL) _pool = new std::map<std::string, BITMAP*>();

    //Cleans the cache.
    CleanCache();

    //Partial Sequential Search for requested BITMAP.
    if(_pool->empty() == false) {
        for(std::map<std::string, BITMAP*>::iterator _iter = _pool->begin(); _iter != _pool->end(); ++_iter) {
            //String keys sorted in ascending order.
            //If key is not empty reached non-blank section.
            if(_iter->first.empty() == false) break;
            if(width == _iter->second->w && height == _iter->second->h) {
                return _iter->second;
            }
        }
    }
    //Attempt to create bitmap, if failed, return NULL.
    BITMAP* result = create_bitmap(width, height);
    if(result == NULL) return NULL;

    //Clear to black, store it, then return to caller.
    clear_bitmap(result);
    _pool->insert(std::pair<std::string, BITMAP*>("", result));
    return result;
}

void BitmapCache::CleanCache() {
    //Clean the pool of any NULL bitmaps that were deleted by caller.
    for(std::map<std::string, BITMAP*>::iterator _iter = _pool->begin(); _iter != _pool->end(); ++_iter) {
        if(_iter->second != NULL) continue;
        _pool->erase(_iter);
    }
}

Answer 1

Ahh, the wonderful world of game design - I assume that is what you are doing. In my oppinion, the three most common solutions are (in order of my preference):

1. Make the cache an object instead of a pointer to an object. This way it will automatically be destructed correctly when your application ends. Disadvantage: No real way to shutdown the game and 'reboot' without restarting the app, but unless you are switching libraries or graphic backends, this is also not really needed.
2. Leave everything as it is, don't worry. I know, the destructor won't run, but upon application exit the OS will reclaim every bit of memory anyway. However, DO NOT DO THIS IF there are 'important' tasks that need to be taken care of during cache destruction, like (i don't know) writing something back to disk. Note that this is only viable if you want to discard the complete cache object (not only its contents) only upon process termination, or else you are going to have serious memory leaks. Also do not do this if the application needs to continue execution for a long time after the your game has quit.
3. In case neither 1) or 2) are viable, you are probably in the need to shut down your 'game engine' and start it back up again without terminating your app. In this case, however you will have some method anyway to 'shut down everything'/'close engine'/'reinit'/call it whatever you want. Then simply, this method can take care of releasing your cache.

Note to solution 2: This 'solution' is dirty and if you are a good coder you really should properly destruct all objects even upon application exit, just like I always do. But to be honest, in this particular case you have almost nothing to gain from it. The only thing that I can think of is, if you are using a memory leak detector, properly terminating everything will spare you thousands of false warnings (which IS important IF you are using a leak detector). If you are not sure, forget about option 2) and do it right.

Answer 2

You initialization is lazy, so there is no need for an explicit Initialize. A Release Method would be one option to clean the memory after usage and setting pool to NULL again. I guess your code is intended for single thread only so there are no real problems. Another option would be to use instances a your (then modified) class. In that way you give the user the control to use different Caches for different usages. The Cache deletes if it goes out of scope (local use) or if the user explicitly wants to.

Personally I think it's rarely a good design to use Classes in a static way. If you find it neccessary to limit the number of instances, why not making it a Singleton instance?

BTW I'd suggest a typedef for std::map<std::string, BITMAP*> and std::pair<std::string, BITMAP*> making the code a bit more readable.

Answer 3

If I'm understanding what you're doing correctly, it seems like the callers should get a shared_ptr to the bitmap, while the cache keeps a weak_ref. That way, bitmaps will only exist while you are using them, and the cache will know when a bitmap stops being used (the weak_ref becomes invalid). However, this does lead to the overhead of shared_ptr, which may be unacceptable in a game.

EDIT:

I'm not sure why you'd new and delete the map; however, you could make people Release() each individual bitmap, and the map would then check for that while cleaning. This will incur very little overhead (one bool, plus a check and occasional delete when searching).

In any case, just letting it be and never unloading bitmaps sounds like a very bad option.