7

I'm currently trying to store a std::unique_ptr in a std::unordered_map, but I get a weird compile error. Relevant code:

#pragma once

#include "Entity.h"

#include <map>
#include <memory>

class EntityManager {
private:
    typedef std::unique_ptr<Entity> EntityPtr;
    typedef std::map<int, EntityPtr> EntityMap;

    EntityMap map;
public:

    /*
    Adds an Entity
    */
    void addEntity(EntityPtr);

    /*
    Removes an Entity by its ID
    */
    void removeEntity(int id) {
        map.erase(id);
    }

    Entity& getById(int id) {
        return *map[id];
    }
};

void EntityManager::addEntity(EntityPtr entity) {
    if (!entity.get()) {
        return;
    }

    map.insert(EntityMap::value_type(entity->getId(), std::move(entity)));
}

This is the compile error:

c:\program files (x86)\microsoft visual studio 12.0\vc\include\tuple(438): error C2280: 'std::unique_ptr<Entity,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
1>          with
1>          [
1>              _Ty=Entity
1>          ]
1>          c:\program files (x86)\microsoft visual studio 12.0\vc\include\memory(1486) : see declaration of 'std::unique_ptr<Entity,std::default_delete<_Ty>>::unique_ptr'
1>          with
1>          [
1>              _Ty=Entity
1>          ]
1>          This diagnostic occurred in the compiler generated function 'std::pair<const _Kty,_Ty>::pair(const std::pair<const _Kty,_Ty> &)'
1>          with
1>          [
1>              _Kty=int
1>  ,            _Ty=EntityManager::EntityPtr
1>          ]
user3125280
  • 2,779
  • 1
  • 14
  • 23
Tips48
  • 745
  • 2
  • 11
  • 26
  • compiles fine with clang++ 3.5 – Ryan Haining Jan 11 '14 at 00:28
  • btw, that's not an `unordered_map`, it's just a `map` – Ryan Haining Jan 11 '14 at 00:29
  • whoops, i changed it from a `map` to an `unordered_map` after copying this – Tips48 Jan 11 '14 at 00:31
  • 3
    I don't know about the compile-time error, but this call: `map.insert(EntityMap::value_type(entity->getId(), std::move(entity)))` would be unsafe anyway because it's unspecified whether the move of `entity` into the function's argument would occur before or after the evaluation of `entity->getId()`. Assign `getId()` to a temporary to pass to the function. – Michael Burr Jan 11 '14 at 00:48
  • @MichaelBurr that's a great point – Ryan Haining Jan 11 '14 at 00:49
  • @MichaelBurr: See [this related proposal](https://groups.google.com/a/isocpp.org/forum/#!topic/std-proposals/8hO-6sm3nlc)... – Kerrek SB Jan 11 '14 at 00:51
  • 1
    It is most likely a problem with the `std` library MSVC ships. Look for bug reports, or set up a http://ssccr.org and submit one? – Yakk - Adam Nevraumont Jan 11 '14 at 01:06
  • 1
    You can try: `map.insert(std::move(std::make_pair(entity->getId(), std::move(entity))));` or try using `std::forward`. I had that error before. I don't remember how I solved it in VS2012 but I know for sure it involved a forward or a move.. ANother option is to insert a nullptr into the map. Then using the index operator, move assign to it. – Brandon Jan 11 '14 at 01:07
  • Interestingly enough, one of the two parameters to the `pair` is `const`: maybe the move constructor of MSVC's `pair` fails when this is the case? – Yakk - Adam Nevraumont Jan 11 '14 at 01:23
  • @Tips48 does my answer or CantChooseUsername's work? – user3125280 Jan 11 '14 at 01:45
  • @Yakk seems more an issue of overload selection. There is a valid copy insert funtion. – user3125280 Jan 11 '14 at 01:46
  • @user3125280 I'm suspecting you are calling the `move` insert function. But when the `pair` is `move`d into the final resting spot of the data, the `move` ctor on `pair` fails to match because it contains `const` data. So it falls back on the `copy` ctor, which generates your error. Said `pair` is an intermediate object whose creation is hard to avoid... – Yakk - Adam Nevraumont Jan 11 '14 at 02:05
  • @Yakk the value_type itself always has const key - it is no intermediate - this is not a problem usually is it? (on gcc, etc). the move insert should work when its class argument (templated) can be used to construct a value_type, which surely is the case for value_type&& since there is a move constructor. that said, the compiler should favour the copy constructing (non-template) overload since it is an exact match here.. i'm very confused – user3125280 Jan 11 '14 at 02:25
  • @Yakk [here is proof](http://ideone.com/5FhRRo) that the move ctor is valid - someone should test it on vs2012 – user3125280 Jan 11 '14 at 02:33
  • I am only talking about msvc half finished c++11 -- theorizimg where it might be broken. @user3125280 – Yakk - Adam Nevraumont Jan 11 '14 at 03:25
  • @Yakk i put some test code for your theory in my question – user3125280 Jan 11 '14 at 03:58
  • @Tips48 : You didn't post the relevant code (the code calling addEntity) _or_ the relevant errors (errors for the code calling pair's copy c'tor)... – ildjarn Jan 11 '14 at 04:52
  • @ildjarn the code calling the copy ctor is compiler generated, and is caused by the call to map::insert (not addEntity) and the error is unfortunately all the information OP appears to have. See my answer – user3125280 Jan 11 '14 at 07:10
  • Can someone explain the point of passing a UniquePtr by reference to add it? The point of unique pointer is to only have one, why not to just pass a raw pointer and the `addEntity` wraps it automatically (like how @RyanHaining suggested it)? If you are planning on using the pointer with multiple owners, use shared_ptr instead and pass it by reference. – Gasim Jan 11 '14 at 13:58
  • 1
    It's passed by rvalue reference, which will transfer ownership from the temporary to the map. that's what the move constructor is for. ex: `std::unique_ptr up(new int); std::unique_ptr up2(std::move(up));` ownership of the int pointer is transferred from `up` to `up2` – Ryan Haining Jan 11 '14 at 19:07
  • My compiler sees no problem: GNU C++ (i686-posix-dwarf-rev1, Built by MinGW-W64 project) version 4.9.2 (i686-w64-mingw32) – Roland Sep 13 '16 at 12:08

4 Answers4

7

The error is because somewhere in the code, map wants to copy a std::pair<int, std::unique_ptr<Entity>>, however there is no copy constructor capable of this, because unique_ptr's are not copy constructable. This is specifically impossible to prevent multiple pointers owning the same memory.

So before std::move, there was no way to use an uncopiable element.

There are some solutions here.

However, in c++11 Map can make use of std::move to work with non-copyable values.

This is done by providing another insert operator, which is overloaded to include this signature:

template< class P > std::pair<iterator,bool> insert( P&& value );

This means an rvalue of a class that can be turned into a value_type can be used as an argument. The old insert is still available:

std::pair<iterator,bool> insert( const value_type& value );

This insert actually copies a value_type, which would cause an error since value_type is not copy constructable.

I think the compiler is selecting the non-templated overload, which causes the compilation error. Because it is not a template, it's failure is an error. On gcc at least, the other insert, which uses std::move, is valid.

Here is test code to see if your compiler is supporting this correctly:

#include <iostream>
#include <memory>
#include <utility>
#include <type_traits>

class Foo {
};

using namespace std;

int main() {
    cout << is_constructible<pair<const int,unique_ptr<Foo> >, pair<const int,unique_ptr<Foo> >& >::value << '\n';
    cout << is_constructible<pair<const int,unique_ptr<Foo> >, pair<const int,unique_ptr<Foo> >&& >::value << '\n';
}

The first line will output 0, because copy construction is invalid. The second line will output 1 since the move construction is valid.

This code:

map.insert(std::move(EntityMap::value_type(entity->getId(), std::move(entity))));

should call the move insert overload.

This code:

map.insert<EntityMap::value_type>(EntityMap::value_type(entity->getId(), std::move(entity))));

Really should call it.

EDIT: the mystery continues, vc returns the incorrect 11 for the test...

Community
  • 1
  • 1
user3125280
  • 2,779
  • 1
  • 14
  • 23
  • Using GCC on Linux with `-std=c++14`, I don't need the outer call to `std::move()`. With or without the outer call to `std::move()`, it works. Also, you *may* need keyword `typename` in front of `EntityMap::value_type` (depending on your template hierarchy). If so, consider using defining a local `typedef`; helps to improve code readability. – kevinarpe May 03 '15 at 12:48
  • I cannot believe how hard it is to find C++ code examples via Google demonstrating `std::unique_ptr` in action with `std::move()` and `std::map`. Incredible. This answer was so important to fixing a syntax error in my code. – kevinarpe May 03 '15 at 12:49
  • The key takeaway from this answer: Avoid calling `std::make_pair()` and instead directly construct instances of `EntityMap::value_type`. – kevinarpe May 03 '15 at 12:52
  • @kevinarpe the problem with map/set and non-copyables like `unique_ptr` is that when used as keys in those containers, they are accessed only as const, and there are some bumps in c++ that make const non-copyable objects *impossible* to remove (in a non ub way) - if they are moved from without being edited (they're const), the destructor will be called once for the object in the container, and once for the moved copy (identical, because of const). Ie a smart pointer would deallocate twice, etc. I asked about that [here](http://stackoverflow.com/questions/21085735/are-move-semantics-incomplete) – user3125280 May 03 '15 at 13:24
  • @kevinarpe i think make_pair should be fine, since a temp will be bind to universal reference as r value - but i guess it might call a different overload if you don't wrap it in move – user3125280 May 03 '15 at 13:27
  • I can't make the above code work (or something very similar) when using `std::make_pair()`. It is also possible the errors are due to non-const correctness. – kevinarpe May 04 '15 at 08:36
1

Your code works with the following:

int main() {
    EntityManager em;
    em.addEntity(std::unique_ptr<Entity>(new Entity(1)));

    return 0;
}

However this is cumbersome and I'd recommend defining addEntity like so:

void EntityManager::addEntity(Entity *entity) {
    if (entity == nullptr) 
        return;
    }

    map.insert(EntityMap::value_type(entity->getId(),
                std::unique_ptr<Entity>(entity)));
}

and inserting with

em.addEntity(new Entity(...));
Ryan Haining
  • 35,360
  • 15
  • 114
  • 174
  • This is how I add the entity. Customer extends Entity. `entityManager.addEntity(std::unique_ptr(new Customer(this, Vec2D(200, 200))));` – Tips48 Jan 11 '14 at 00:36
  • @Tips48 idk what it could be, or where exactly it's coming from. You'll have to post an example that compiles to the error you have. – Ryan Haining Jan 11 '14 at 00:42
  • That's the thing, it doesn't give a line number – Tips48 Jan 11 '14 at 00:43
  • the error is with the copy constructor - it is called somewhere in code despite unique_ptr not being copy constructable - hence referecne to 'deleted' function (the copy constructor) – user3125280 Jan 11 '14 at 00:43
  • i don't have said compiler/platform available, however I assume Ty is the value type, KTy is the key type, so some where when some function returns a pair of key and value, it is calling the copy constructor (your own call to create such a pair is valid since you use std::move) - let me think about this for a second... – user3125280 Jan 11 '14 at 00:47
  • @Tips48 if you make the paremeter of addEntity `EntityPtr&&` instead of `EntityPtr` it will likely reveal the culprit that's passing an lvalue unique_ptr to be copied – Ryan Haining Jan 11 '14 at 00:47
  • @Tips48 the other answer says your move isn't working - maybe that is the culprit? try getting rid of it – user3125280 Jan 11 '14 at 00:48
  • The above answer didn't help. What were you thinking? – Tips48 Jan 11 '14 at 00:56
  • @Tips48 The only way I can see a copy contructor being invoked is if someone passed an lvalue unique_ptr to your function rather than an lvalue. If you made the function take an rvalue reference then the call itself would fail because the argument wouldn't match – Ryan Haining Jan 11 '14 at 01:11
  • @RyanHaining no the copy constructor is for pair - the type used internally to map. the pair copy constructor is based on the copy of both elements, one of which (the unique_ptr) is not copiable – user3125280 Jan 11 '14 at 01:12
  • @Tips48 may I ask why you are you using a `unique_ptr` at all instead of just a map of id -> entity? – Ryan Haining Jan 11 '14 at 01:14
  • @RyanHaining presumably, he doesn't want entity copied. Still, raw pointers would work. – user3125280 Jan 11 '14 at 01:16
  • @user3125280 could make entity itself movable and non-copyable, but that would be the same problem with pair copying it. are you also thinking this is MSVC misbehaving? – Ryan Haining Jan 11 '14 at 01:21
  • @RyanHaining i think so - it is acceptable c++03 behaviour, but move should fix this really – user3125280 Jan 11 '14 at 01:22
1

I had the same issue on VS 2017 with msvc 14.15.26726. According to the compiler error log, things seem to be related to the need for a copy ctor for std::pair<_kT, _T> during instantiation. I don't know why, but one interesting observation (and workaround) for me is to put a declaration of a std::unique_ptr before the declaration of the map, e.g.:

#pragma once

#include "Entity.h"

#include <map>
#include <memory>

class EntityManager {
private:
    typedef std::unique_ptr<Entity> EntityPtr;
    typedef std::map<int, EntityPtr> EntityMap;
    std::unique_ptr<Entity> aDummyStub; //<-- add this line
    EntityMap map;
//...
};
DXZ
  • 451
  • 4
  • 14
0

Not sure if this solution could help you as well, but I suddenly got the same error on a private std::map<int, std::unique_ptr<SomeType>> data member when I switched from a static library to a dynamic library in Visual Studio 2015 (Update 2).

Since using template data members together with __declspec(dllexport) produces a warning (at least in MSVC), I resolved that warning by (almost) applying the PIMPL(Private Implementation) idiom. Surprisingly, the C2280 error also disappeared that way.

In your case, it would be:

class EntityManagerPrivate {
public:
    EntityMap map;
};

class EntityManager {
private:
    EntityManagerPrivate* d; // This may NOT be a std::unique_ptr if this class 
                             // shall be ready for being placed into a DLL
public:

    EntityManager();
    ~EntityManager();

   // ...
};

and in the .cpp file:

EntityManager::EntityManager() :
    d( new EntityManagerPrivate() )
{
}

EntityManager::~EntityManager()
{
    delete d;
    d = nullptr;
}

// in all other methods, access map by d->map

Note that for a real PIMPL you would have to move the private class into an own header file which is only referenced by the .cpp. The actual header would only have a forward declaration class EntityManagerPrivate; after the includes. For a real PIMPL, the private class would also have to have implementation in addition to data members.

Tim Meyer
  • 12,210
  • 8
  • 64
  • 97