3
\$\begingroup\$

I have an SDL_Wrapper which is working perfectly (it is not broken)! Please suggest how I could improve performance, and how I could make my unique_ptr(s) dispose automatically.

So far, I call my class like this:

CWindow window = std::make_unique<CWindowWrap>("title", 0, 0, 140, 100, 0)

Is it the best way to do it? Will my unique_ptr dispose automatically?

Here's the SDL_Wrapper class:

 #include "LogManager.h"
 #include <memory>
 #include <SDL.h>


 //SDL_Renderer wrapper

 ////SDL_Window wrapper !


 class CWindowWrap
 {

 public:
     CWindowWrap(const char* title, int xpos, int ypos, int width, int height, int flags)
         : ptr_window(SDL_CreateWindow(title, xpos, ypos, width, height, flags))
     {
        LOG("SDL_Window Wrapper", "Constructed Window !");
    }
     virtual ~CWindowWrap()
     {
         if (ptr_window != nullptr){
            SDL_DestroyWindow(ptr_window);
            LOG("SDL_Window Wrapper", "Destroyed Window !");
        }
        else 
             LOG("SDL_Window Wrapper", "Window doesn't need to be destroyed !");
     }

     //disable copy constructor
     CWindowWrap(CWindowWrap const&) = delete;
    CWindowWrap& operator=(CWindowWrap const&) = delete;

    //allow move
    CWindowWrap(CWindowWrap&& move)
        : ptr_window(nullptr)
    {
         using std::swap;
         swap(ptr_window, move.ptr_window);
    }

     CWindowWrap& operator=(CWindowWrap&& move){
         using std::swap;
         swap(ptr_window, move.ptr_window);
     }

     operator SDL_Window*() { return ptr_window; } // implicite conversion between CWindowWrap ---> SDL_Window*;

 private:
     SDL_Window* ptr_window = nullptr;
 };

 typedef std::unique_ptr<CWindowWrap> CWindow;


 class CRendererWrap
 {
 public:

     CRendererWrap(CWindowWrap window, int x, int y)
         : ptr_renderer(SDL_CreateRenderer(window, x, y))
     {}
    ~CRendererWrap()
     {
        if (ptr_renderer != nullptr){
            SDL_DestroyRenderer(ptr_renderer);
        }
     }

     //disable copy constructor
     CRendererWrap(CRendererWrap const&) = delete;
     CRendererWrap& operator=(CRendererWrap const&) = delete;

     //allow move
     CRendererWrap(CRendererWrap&& move)
        : ptr_renderer(nullptr)
     {
        using std::swap;
        swap(ptr_renderer, move.ptr_renderer);
     }

     CRendererWrap& operator=(CRendererWrap&& move){
        using std::swap;
        swap(ptr_renderer, move.ptr_renderer);
    }

     operator SDL_Renderer*() { return ptr_renderer; } // implicite conversion between CRendererWrap ---> SDL_Renderer*;

 private:
      SDL_Renderer* ptr_renderer = nullptr;
 };

 typedef std::unique_ptr<CRendererWrap> CRenderer;


 struct Window_param
 {
     const char* title;
     const int width;
     const int height;
     const int xPos;
     const int yPos;
     const int flags;
 };
\$\endgroup\$
5
\$\begingroup\$

I think that conceptually you're fine. There are some details that need attention.

Hide the implementation

Your creation syntax:

CWindow window = std::make_unique<CWindowWrap>("title", 0, 0, 140, 100, 0)

requires the user to know that CWindow is a unique_ptr<CWindowWrap> this isn't ideal. I believe that it's a good idea to hide the implementation class.

You should put your CWindowWrap class in an implementation namespace:

namespace detail{
    class CWindowWrap{
        ...
    };
}
using CWindow = std::unique_ptr<detail::CWindowWrap>;

Then you should provide a creator function like this:

CWindow make_window(const char* title, int xpos, int ypos, int width, int height, Uint32 flags){
     return std::make_unique<detail::CWindowWrap>(title, xpos, ypos, width, height, flags);
}

Don't use virtual if you don't need it

CWindowWrap doesn't need a virtual destructor as far as I can tell and I can't imagine why you would inherit from this. So simply remove virtual.

Wrapper classes should match types exactly

You're not matching the types properly for the constructor of CWindowWrap (flags should be Uint32 see here).

Move assignment/construction

As you have already initialized ptr_window in the in-class declaration, you do not need : ptr_window(nullptr) in your move constructor.

Your move assignment operator is missing a return statement.

That said, as you are not supposed to instantiate CWindowWrap directly you could simply = delete the move assignment operator and constructor. They won't be used.

Same comments goes for the other wrapper.

\$\endgroup\$
  • \$\begingroup\$ Thanks : ); but do you have an idea why my unique_ptr doesn't gets deleted automaticaly ? I must call delete ptr.get() to call the destructor, which really is a pain... \$\endgroup\$ – MattMatt Apr 26 '15 at 12:21
  • \$\begingroup\$ When the unique_ptr containing the object gets destructed, it will call delete on the contained pointer. If you have created another unique_ptr from the original, the pointer will have been transferred from the old to the new. And your destructor will not be called when the old one is destructed, but when the new one is. If this behavior isn't desired, you should look at std::shared_ptr which I think is what you actually want. \$\endgroup\$ – Emily L. Apr 26 '15 at 12:46
  • \$\begingroup\$ Thanks :), but I tried shared_ptr and it won't get destroyed too ! I just use 1 pointer, and as you can see, when my object is destroyed, I have a log message appearing ! but unfortunatly, it doesn't appear; This isn't really frustrating for me, but I'm building an SDL framework, so if my users think that their objects get destroyed automatically, there will be big problems ! \$\endgroup\$ – MattMatt Apr 26 '15 at 12:59
  • \$\begingroup\$ Thank you !!!!! I've figured it out ! Now everything gets disposed after the program shuts down ! One more question : I've made special functions like : construct _renderer, and construct_window; shoulod I make the Window and Renderer Constructor private, and make those functions friend of the class ? \$\endgroup\$ – MattMatt Apr 26 '15 at 16:21
  • \$\begingroup\$ Also, One little thing : When I try to delete my shared_ptr by calling delete window.get(), I get an error saying that my programs tries to delete a memoy location that was already deleted ! I think that he shared_ptr tries to delete it after the end of my program : what shall I do ? \$\endgroup\$ – MattMatt Apr 26 '15 at 16:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.