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;
};
1 Answer 1
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.
-
\$\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– MattMatt2015年04月26日 12:21:34 +00:00Commented Apr 26, 2015 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 anotherunique_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 atstd::shared_ptr
which I think is what you actually want. \$\endgroup\$Emily L.– Emily L.2015年04月26日 12:46:36 +00:00Commented Apr 26, 2015 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– MattMatt2015年04月26日 12:59:00 +00:00Commented Apr 26, 2015 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– MattMatt2015年04月26日 16:21:41 +00:00Commented Apr 26, 2015 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– MattMatt2015年04月26日 16:48:46 +00:00Commented Apr 26, 2015 at 16:48