C++ std::unique_ptr in std::map

Question

Is this:

std::unique_ptr<sf::Texture> texture;
texture = std::move(textureMap.at(id));  
return *texture;

the same like this?

auto found = textureMap.find(id);
return *found->second;

textureMap is:

std::map<ID, std::unique_ptr<sf::Texture>>

If yes, what's better solution?

Thanks!

Answer 1

The first one moves the pointer out of the map, so that the object it points to is destroyed when the function returns. If the function returns a reference, then that reference will be invalid. It also leaves an empty pointer in the map, which will cause problems later if you try to dereference it - the code assumes the pointers are not empty, and so will have undefined behaviour if they are.

The second leaves the pointer where it is, so the object survives. If the function returns a reference, then that reference is still valid. However, it has undefined bahaviour if the key is not found; it should either check that found != textureMap.end(), or return *textureMap.at(id) which will throw if the key isn't found.

Answer 2

If you would like to avoid copying heavy texture objects, you can make sf::Texture movable but non-copyable, e.g.:

struct Texture
{
    Texture(Texture const&) = delete;
    Texture& operator=(Texture const&) = delete;

    Texture(Texture&&) = default;
    Texture& operator=(Texture&&) = default;
};


int main() {
    std::map<int, Texture> texture_map;
}

Achieves the same effect as std::unique_ptr<> but with less complexity.

Answer 3

Strictly speaking apart from both being wrong, they are not equivalent. The first version can throw an std::out_of_range exception, while the second one will invoke an undefined behaviour if the key is not found.

Answer 4

Both versions are bad.

The first version first unnecessarily creates a new sf::Texture. Then it moves a texture out of the texture map, which possibly destroys the original texture inside the map. Then it returns a value that does not exist anymore, because it has been deleted by the unique_ptr.

The second version has the problem that it will crash is UB if the id is not found in the map.

A better version would be something like this

auto found = textureMap.find(id);
if (found != textureMap.end())
    return *found;
throw "not found error";

Answer 5

Both are bad ideas:

1. at throws if key not found, then moves the pointer value (Setting the value of the pair to 0).
2. The second case has Undefined Behavior, bcause you must not dereerence the iterator if key was not found.

What you probably wanted is this:

auto found = textureMap.find(id);
if(found == textureMap.end())
    throw std::out_of_range();
return *found->second;

That is equal to *map.at() btw.