0

This is not homework, just a question about my code (I'm learning C++).

I have multiple instances of the class Renderer that all use the same resource, a BMP loaded by SDL. Is this a correct way to manage a shared resource for a class? If not, what is a good one? If yes, are there better ones?

renderer.hpp

class Renderer {
    public:
        Renderer(SDL_Surface *target_surface, int w, int h);
        Renderer(const Renderer& renderer);
        ~Renderer();
        // ...
    private:
        int w, h;
        SDL_Surface *target;
        
        static SDL_Surface *blocks;
        static int numinstances;
};

renderer.cpp

const char BLOCKS_FILE[] = "blocks.bmp";
SDL_Surface *Renderer::blocks = 0;
int Renderer::numinstances = 0;

Renderer::Renderer(SDL_Surface *target, int w, int h) {
    numinstances++;
    
    if (blocks == 0) {
        // temporary storage for file
        SDL_Surface *loadedimg = SDL_LoadBMP(BLOCKS_FILE);
        if (loadedimg != NULL) {
            blocks = SDL_DisplayFormat(loadedimg);
            SDL_FreeSurface(loadedimg);
        }
    
    }

    this->target = target;
    this->w = w;
    this->h = h;
}

Renderer::Renderer(const Renderer& renderer) {
    numinstances++;
    
    w = renderer.w;
    h = renderer.h;
    target = renderer.target;
    
}

Renderer::~Renderer() {
    numinstances--;
    
    if (numinstances == 0) {
        SDL_FreeSurface(blocks);
        blocks = 0;
    }
}
Community
  • 1
  • 1
orlp
  • 112,504
  • 36
  • 218
  • 315

1 Answers1

2

Is this a correct way to manage a shared resource for a class?

Yes, strictly speaking it's a correct way. But it's one that you should walk away from as fast as you can. No, don't walk - run. And don't look back.

If not, what is a good one?

Prefer anything that resembles std::shared_ptr<> or boost::shared_ptr<>.

If yes, are there better ones?

Instead of having a static surface pointer and a static reference counter inside the class, just keep one shared_ptr for the surface there, create it once outside of the class and then pass it to the renderer constructor. Some benefits of doing so:

(Edit in bold:)

  • You don't need to think about who's the last owner and thus responsible for resource deletion, since shared_ptr does that for you.
  • Thread safety
  • Less code often means fewer bugs.
Johann Gerell
  • 24,991
  • 10
  • 72
  • 122
  • Advising people "not to think" about object lifetimes is not good, IMHO - shared_ptr is not the answer to every problem. –  May 05 '11 at 17:30
  • @unapersson: Sigh. I guess I shortcut-ed the sentence a bit too prematurely. See my edit. – Johann Gerell May 05 '11 at 17:52