How to wrap a list of pointers to abstract class?

Question

I try to wrap a list of smart pointers to abstract class (list<shared_ptr<Base>> list_) into some classes (Item, Drawer, Box). Then in the main function, I have a map of Box'es and it doesn't work. I found a way around and I can use new but I suspect that it just causes bugs which I can't see. How to make it work? Here is the code:

#include <iostream>
#include <list>
#include <map>
#include <memory>
using namespace std;

class Base {
public:
    virtual int get() = 0;
};

class Derived : public Base {
public:
    Derived(int x) { x_ = x; }
    int get() override { return x_; }
private:
    int x_;
};

class Item {
public:
    Item() {
        for (int i = 1; i <= 10; i++) {
            list_.push_back(make_shared<Derived>(i));
        }
    }
    list<shared_ptr<Base>>& get_list() { return list_; }
private:
    list<shared_ptr<Base>> list_;
};

class Drawer {
public:
    Drawer(Item& item) : item_(item) {}
    void Draw() {
        list<shared_ptr<Base>>& list = item_.get_list();
        cout << list.size() << ":  ";
        while (!list.empty()) {
            shared_ptr<Base> pointer = dynamic_pointer_cast<Derived>(list.front());
            cout << pointer->get() << " ";
            list.pop_front();
        }
        cout << endl;
    }
private:
    Item& item_;
};

class Box {
public:
    Box() : drawer_(item_) {}
    void Draw() { drawer_.Draw(); }
private:
    Item item_;
    Drawer drawer_;
};

int main() {
    Box box;
    box.Draw();

    map<int, Box> boxes;                                // it doesn't work, why?
    for (int i = 0; i < 3; i++) {
        boxes.insert(std::pair<int, Box>(i, Box()));
    }
    for (auto& b : boxes) { b.second.Draw(); }

    map<int, Box*> pointers;                            // it does work, why?
    for (int i = 0; i < 3; i++) {
        pointers.insert(std::pair<int, Box*>(i, new Box()));
    }
    for (auto& b : pointers) {  b.second->Draw(); }
    for (auto& b : pointers) {  delete b.second; }
}

And here is the result:

10:  1 2 3 4 5 6 7 8 9 10
0:
0:
0:
10:  1 2 3 4 5 6 7 8 9 10
10:  1 2 3 4 5 6 7 8 9 10
10:  1 2 3 4 5 6 7 8 9 10

Answer 1

In this line here

boxes.insert(std::pair<int, Box>(i, Box()));

You are creating a temporary Box object in your pair, which is moved into the map.

Let's call them Box1, the temporary that is created, and Box2, the move-constructed object inside the map.

When Box1 is created it correctly has a drawer which refers to the item in Box1.

When we then move it into the map, we get Box2 that has a drawer that still refers to the item in Box1.

When we then go on

for (auto& b : boxes) { b.second.Draw(); }

Box1 has already been destroyed and doesn't exist anymore. So when we try to use the reference to it we are using a dangling reference which is UB. In this case you get the result of 0, but you could equally get a crash or any random output.

To fix it we can add a copy constructor to Box to deal with this.

class Box {
public:
    Box() : drawer_(item_) {}
    Box(const Box& other) : item_(other.item_), drawer_(item_) {}
    void Draw() { drawer_.Draw(); }
private:
    Item item_;
    Drawer drawer_;
};

Now the drawer of the copy will refer to the right item.

As to why the version with pointers work, it since we are copying the pointer, so the same object remains until it's deleted. There is no object moved or copied, only the pointer is copied, and a copied pointer still refers to the right object.

Answer 2

 Box() : drawer_(Drawer(item_)) {}

You have created a Drawer(item_) object and then calling the copy constructor for drawer_(). A default copy constructor doesn't always handle complex data structures.

Try

Box() : drawer_(item_) {}

to call the normal constructor for Drawer