CODEWITHSUNDEEP
c++c++11c++14
Anakin
Anakin

Reputation: 2010

Error in pushing unique_ptrs from map into a vector

I have a std::map of std::string and std::unique_ptr<BaseInt>. Basically I want to have a map with the class name as string key and a unique pointer as the corresponding value. And access the pointer as map["Derived1"] etc (explained in code below).

When I iterate over the std::map and try to push each value to a std::vector, I see the following error

Error C2280 'std::unique_ptr<BaseInt,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)': attempting to reference a deleted function CreateInstanceFromList c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\xmemory

I am on Visual Studio 2017 Version 15.9.16 MSVC 14.16.27023

The implementation code is as follows. BaseInt is a BaseClass with an int member and a pure virtual replaceInt(). DerivedInt1 and DerivedInt2 implement the virtual function with different int values and differ by a parameter in their construction.

#include "UserClass.h"
#include <iostream>
#include <vector>
#include <map>
#include <string>
#include <memory>
#include <typeinfo>

typedef std::vector<std::unique_ptr<BaseInt>> vec_type;
typedef std::map<std::string, std::unique_ptr<BaseInt>> map_type;

template<typename T> std::unique_ptr<T> createInstance(vec_type& vec) { return std::make_unique<T>(); };
template<typename T, typename U> std::unique_ptr<T> createInstance(vec_type& vec, U u) { return std::make_unique<T>(u); };

void fillVector(map_type& map)
{
    vec_type my_vec;
    for (auto const& it : map )
    {
        std::cout << it.first << std::endl;
        it.second->replaceInt();

        //my_vec.emplace_back(std::move(it.second)); //this line gives error
    }
    // idea is to be able to access the pointer as map["Derived1"]
    std::cout << my_vec.size() << std::endl;
}

int main()
{
    map_type my_map;
    
    my_map.emplace("Derived1", createInstance<DerivedInt1>(my_vec, 7));
    my_map.emplace("Derived2", createInstance<DerivedInt2>(my_vec));

    fillVector(my_map);

    return 0;
}

My intuition is somehow I am trying to call the copyconstructor of the unique_ptr but I don't actually see how. Thanks.

Edit:

So, the main problem is the const& iterator as @ALX23z mentioned in the answer. The following change works:

    for (auto it = map.begin(); it != map.end(); ++it )
    {
        std::cout << it->first << std::endl;
        it->second->replaceInt();

        my_vec.emplace_back(std::move(it->second));
    }

Edit 2:

As pointed out by multiple people, I made a basic design flaw of not using unique_ptr as unique. I can see the problem you mentioned and I will look into the possibility of changing to a shared_ptr or modifying the design. Thanks for all the quick response. I will update my changes here tomorrow.

Edit 3:

After looking at the main project scenarios, I can see that the std::map here is really just a one-time use container, so I can keep it local and pass all ownership to the std::vector.

Edit Again:

Someone downvoted the question and flagged to close it mentioning

Questions seeking debugging help ("why isn't this code working?") must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself. Questions without a clear problem statement are not useful to other readers. See: How to create a Minimal, Reproducible Example.

I really would like them to enlighten me on how this flagging was deemed necessary. I totally presented a short code necessary to reproduce the exact problem I was facing; instead of the huge project code I am actually working on. I provided all information necessary with even my intuition of where it was failing. I added multiple edit information so that any reader can find it useful. Atleast, leave a comment and explain what can be improved. Sorry for the rant.

Upvotes: 0

Views: 491

Answers (2)

stefanB
stefanB

Reputation: 79780

std::unique_ptr is the owner of the pointer, it'll delete the data when you destroy the std::unique_ptr.

So you don't want to have two std::unique_ptr's to point to the same data because when one of them deletes your data the other one ends up pointing to something which is no longer your data. You could call std::unique_ptr<>::release() before deleting the std::unique_ptr but then it sounds like you are not using the std::unique_ptr correctly if you want two "unique pointers".

You could use std::shared_ptr. Or, what I usually do, decide who owns the data, vector or map, then the owner stores std::unique_ptr's to data and the other stores just plain pointers. You have to make sure you deal with all the pointers before you delete the std::unique_ptr.

So for example:

typedef std::vector< BaseInt * > vec_type;
my_vec.push_back(it.second.get());

I haven't tried compiling but std::unique_ptr<>::get() returns raw pointer that is still managed by the std::unique_ptr in the map. Then remove from vector before removing from map, because removing from map will delete data pointed to by std::unique_ptr.

EDIT:

When you move the std::unique_ptr the ownership is transfered to the new std::unique_ptr so the old one no longer holds valid pointer, after you move the pointer from map to vector the map no longer has the pointer to your data:

my_vec.emplace_back(std::move(it->second));

Check what you have in map after you move pointers to vector.

Upvotes: 3

ALX23z
ALX23z

Reputation: 4713

The line gives error because you iterate over the map via a const iterator. To apply emplace_back on you need a non-const reference.

Also you have an error with your approach in general. unique_ptr is a unique pointer. You cannot have more than one unique_ptr pointing to the same object. So you cannot share them between map and a vector.

Use 2 shared_ptr, or unique_ptr with a raw pointer depending on requirements.

Upvotes: 3

Related Questions