Doubt about make_shared and move

1) It's not safe to copy or move a SDL_Texture. Instead you should pass the pointer as argument to the shared_ptr constructor (or to the reset function). You also need to specify a custom deleter so that SDL_DestroyTexture will be used instead of delete to free the texture.

I found this page where they do it with SDL_Surface. It should be very similar for SDL_Texture.
http://stackoverflow.com/questions/12340810/using-custom-deleter-with-stdshared-ptr

2) You do not need to call reset(). The old pointee will be automatically deleted when you assign a new value to the shared_ptr.

3) shared_ptr<SDL_Texture*> is like having a SDL_Texture** (a pointer to a pointer to a texture). The default deleter would destroy the pointer but not the SDL_Texture. With a custom deleter you can of course make it work but it looks like it just complicates things without having any benefits.

Last edited on

So if I'm getting it right, two of the ways of doing this properly are

1)

bool TextureManager::Load(std::string fileName, std::string id, SDL_Renderer* pRenderer)
{
//code...
//Store in map
	if (pTexture != 0)
	{
		textureMap[id] = std::make_shared<SDL_Texture>(pTexture);
		return true;
	}
//code...
}

TextureManager::~TextureManager()
{
	for (auto &e : textureMap)
	{
		SDL_DestroyTexture(e.second.get());
	}
}

And the other is
2)

1
2

textureMap[id] = std::make_shared<SDL_Texture>(SDL_CreateTextureFromSurface(pRenderer, pSurfaceTemp),
                                                  [](SDL_Texture *pT) {SDL_DestroyTexture(pT); });

I've got this right?

Last edited on

No, you should not use make_shared.

Instead use

textureMap[id] = std::shared_ptr<SDL_Texture>(pTexture, SDL_DestroyTexture);

or just

textureMap[id].reset(pTexture, SDL_DestroyTexture);

Question 1:
*pTempTexture uses the built-in indirection operator, and so it's an lvalue expression. You're right to explicitly move from it, since it is not temporary.
It probably doesn't do you any good because SDL_Texture is POD (SDL is a C library), but it is correct C++ code unless the SDL says otherwise (which it might).

Question 2:
The explicit call to reset() is unnecessary.
*** Why did you decide you need shared ownership?

Also, by default, the smart pointers assume they manage memory allocated with new; they will delete their managed resource. You can avoid this by providing a custom deleter. Since std::shared_ptr type-erases it, you can give the constructor the corresponding function/functor:

1
2

std::shared_ptr<SDL_Texture>{SDL_CreateTextureFromSurface(pRenderer, pSurfaceTemp),
                             SDL_DestroyTexture};

For std::unique_ptr, the deleter is part of the type. I use this:

  /** \brief Generic deleter functor for SDL resources. For use with std::unique_ptr.
   */
  struct SDLDeleter {
    /* Generate "free" calls. */
# if ! defined FREE_ME
#   define FREE_ME(type, fn_name) void operator()(type * const thing)     \
      const { if (thing) SDL_##fn_name(thing); }
    FREE_ME(SDL_RWops, FreeRW)             FREE_ME(SDL_cond, DestroyCond)
    FREE_ME(SDL_Cursor, FreeCursor)        FREE_ME(SDL_PixelFormat, FreeFormat)
    FREE_ME(SDL_mutex, DestroyMutex)       FREE_ME(SDL_Palette, FreePalette)
    FREE_ME(SDL_Renderer, DestroyRenderer) FREE_ME(SDL_sem, DestroySemaphore)
    FREE_ME(SDL_Surface, FreeSurface)      FREE_ME(SDL_Texture, DestroyTexture)
    FREE_ME(Uint8, FreeWAV)                FREE_ME(SDL_Window, DestroyWindow)
#   undef FREE_ME
# else 
#   error "FREE_ME already defined"
# endif
  };

Like this:

1
2

template <typename T> 
using SDL_unique_ptr = std::unique_ptr<T, SDLDeleter>;

Question 3:

Let's say I use make_shared to store *SDL_Texture instead of SDL_Texture inside my map

You're not storing SDL_Texture nor SDL_Texture* in your map. You're storing std::shared_ptr<SDL_Texture>.

make_shared in this case is not useful because you cannot supply a custom deleter. You need to use the std::shared_ptr constructor to do that.
make_shared allocates its' memory with new and then forwards its' arguments to the constructor of the newly managed resource. In this case, it move-constructs a SDL_Texture in situ. The result of std::make_shared manages that memory.

Last edited on

Peter I'm kind of confused, I thought you where supposed to use shared_ptr and make_shared together, can you explain me what I am not getting here? :S
Edit: I didn't had refreshed the page, reading now mbozzi reply, my answer could be in there :P

Last edited on

It is good to use make_shared when you want to create the object using new, but in this situation the SDL_Texture object has already been created by the SDL_CreateTextureFromSurface function so you can't use make_shared.

Thank you guys for your replies, I need to read them over and over again and also go on youtube and see all the video I can find on smart pointers, because Clearly I have major confusion

*** Why did you decide you need shared ownership?

Don't ask me, I have no idea of what I am doing, the logic behind it would be that I choose it because I don't know that I don't need it...xD

Me right now -> http://i.imgur.com/lTnm4bR.png

That SDLDeleter structure is still too arcane for me to grasp, I will need to investigate that too :P

Last edited on

Me right now

lol

That SDLDeleter structure is still too arcane for me to grasp

You probably know this already, but the best way to understand code that uses macros (of any kind) is to expand the macro to see what the generated code looks like.
The preprocessor generates code that looks like this:

struct SDLDeleter {
  void operator()(SDL_RWops * const thing) const { 
    if (thing) SDL_FreeRW(thing); 
  }
  void operator()(SDL_cond * const thing) const { 
    if (thing) SDL_DestroyCond(thing); 
  }

  /* ...etc... */

  void operator()(Uint8 * const thing) const { 
    if (thing) SDL_FreeWAV(thing); 
  }
  void operator()(SDL_Window * const thing) const { 
    if (thing) SDL_DestroyWindow(thing); 
  }
};

Are you familiar with overloaded functions (AKA static dispatch)? Overloaded operators?

The result of the preprocessor program is a bunch of overloads for functions named operator() (i.e., the function call operator), each of which takes a pointer to a particular kind of SDL resource (e.g., window/texture/surface, etc.), and deletes it appropriately.

You can call any instance of the class with any SDL resource as an argument, and it will be correctly freed/released/destroyed:
SDLDeleter del;
And than
del(my_renderer_ptr) // frees your SDL_Renderer

The FREE_ME macro is only there to make the code shorter.

Thanks for the explanation @mbozzi, actually I can't tell if I knew of the possibility of overloading operator() multiple times or if I am deluding myself into thinking that I knew it and it is obvious only after I read your explanation... :S
But let's assume I knew it :D

The only thing I don't get now about it is the line 14, # undef FREE_ME
What is the reason/need for undefining it?
And where is that # error "FREE_ME already defined" going?
Is "error" another macro, or pseudocode or a c++ keyword or class I don't know of?
And how that error can ever happen if you undefine FREE_ME every time?

Last edited on

And now I'm trying to have that textureMap work with unique_ptr and I'm getting so many error, I am totally not getting it x_x

So unique_ptr<SDL_Texture> won't work for some reason, I read somewhere that SDL_Texture is an opaque resource or something so I should stick to *, so now I have:

std::map<std::string, std::unique_ptr<SDL_Texture*, void (SDL_Texture*)>> textureMap;

and then

textureMap[id] = std::move(std::unique_ptr<SDL_Texture*, void(SDL_Texture*)>{pTexture, SDL_DestroyTexture});

it just won't work and I can't understand the reason... x_x

Last edited on

I don't know why but std::unique_ptr doesn't seem to work when the deleter type is a function pointer.

You can make it work by wrapping it in std::function.

std::map<std::string, std::unique_ptr<SDL_Texture, std::function<void(SDL_Texture*)>>> textureMap;

When you do it this way you need to specify the deleter for each object. Sometimes it is useful to be able to do that but in your situation it's only a bother because you always want to use SDL_DestroyTexture. It also adds unnecessary overhead because each std::unique_ptr has to store extra information about what deleter you have specified.

If you instead let the deleter type be an empty struct/class that overloads operator(), as previously shown by mbozzi, you get rid of the unnecessary overhead and you don't need to specify the deleter each time.

The SDLDeleter has been written to handle any SDL type. If you want something simpler that only works for SDL_Texture you can use something like

struct SDLTextureDeleter
{
	void operator()(SDL_Texture* texture) const
	{ 
		SDL_DestroyTexture(texture);
	}
};

And then

1
2

std::map<std::string, std::unique_ptr<SDL_Texture, SDLTextureDeleter>> textureMap;
textureMap[id] = std::move(std::unique_ptr<SDL_Texture, SDLTextureDeleter>{pTexture});

Note that you don't really need to use std::move here because the std::unique_ptr object is a temporary object.

textureMap[id] = std::unique_ptr<SDL_Texture, SDLTextureDeleter>{pTexture};

You can make it even shorter by using the reset function.

textureMap[id].reset(pTexture);

Last edited on

#error Is a directive like #define or #if . It tells the preprocessor to fail and spit out an error message.

What is the reason/need for undefining it?

In case the string FREE_ME ever appears below where it was defined, it would be macro-expanded. That would probably break something. At worst it could change the behavior of the program, silently.

Same reason for checking to see if it is defined in the first place -- if we don't, we'll just change the meaning of the macro. Which would probably break things.

how that error can ever happen if you undefine FREE_ME every time

If FREE_ME is not defined already, we'll define it and then un-define it again.
If it is already defined, we'll do nothing except complain about it.

Good practice says that if you have an #if/ifdef/ifndef you should usually pair it with an #else . That's because the pre-processor will silently remove or change code otherwise, and it's important to get an error instead of having a broken program.

Thanks again @mbozzi, makes sense.

I also gave up on using unique_ptr for now, need to learn things one at the time so for now I'll roll with normal pointers so at least I don't get spammed with errors and I can go on following the book :P

Last edited on

Good luck!

@Peter87

I don't know why but std::unique_ptr doesn't seem to work when the deleter type is a function pointer.

It does work -- but void(SDL_Texture*) isn't the way to write the type of a function pointer. That's literally the type of a function, not a pointer to it. The corresponding function pointer (and what you need to give to unique_ptr's template arg.) looks like void(*)(SDL_Texture*).

Still, I have to wonder why the standards committee didn't supply the same interface to both unique_ptr and function. Strange stuff.

Last edited on

mbozzi wrote:
The corresponding function pointer (and what you need to give to unique_ptr's template arg.) looks like `void()(SDL_Texture)`.

I tried that but it complains about the deleter being null. Not sure if this is a bug in GCC or not.

#include <memory>

int main()
{
	// error: static assertion failed: constructed with null function pointer deleter
	std::unique_ptr<int, void(*)(int*)> ptr;
}

I got it to work by explicitly specifying a deleter for the nullptr

#include <memory>

void Deleter(int*){}

int main()
{
	std::unique_ptr<int, void(*)(int*)> ptr{nullptr, Deleter};
}

Marcus wanted the unique_ptr inside a map but that gives you errors when trying to use the subscript operator.

mbozzi wrote:
I have to wonder why the standards committee didn't supply the same interface to both unique_ptr and function. Strange stuff.

It's unfortunate but I think unique_ptr is the way it is because it shouldn't be forced to have any additional overhead compared to raw pointers.

With shared_ptr you already have the overhead of shared and weak reference counters so adding just another pointer for the deleter is probably not as controversial. They could probably have opted for another solution in which you could disable the overhead of custom deleter and weak reference counter if you don't need them but that would make it more complicated to use and could easily lead to incompatible shared_ptr types. Also note that std::make_shared wouldn't work if it were not able to set a custom deleter.

Last edited on

Cubbi (4774)

Peter87 wrote:
Not sure if this is a bug in GCC or not.

Standard requirement: if the type of the deleter is pointer or reference, unique_ptr with such deleter cannot be default-constructed or constructed from just the object pointer: http://eel.is/c++draft/unique.ptr.single.ctor#4

Last edited on