Deewy
Deewy

Reputation: 245

Manage abstract classes in std container

After a lot of research I still don't understand how to deal with an abstract class collection with smart pointers.

Here are the errors I got:

error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = Shape; _Dp = std::default_delete<Shape>]'
   base_ptr s = shapes.front();

error: no matching function for call to 'std::unique_ptr<Shape>::unique_ptr(Shape&)'
   shapes.push(base_ptr(b));

By compiling the minimal code to replicate the error (code online avaiable).

    #include <queue>
    #include <memory>

    class Shape {
    public:
        virtual int getPerimeter() =0;
    };

    typedef std::unique_ptr<Shape> base_ptr;

    class Circle : public Shape {
    public:
        virtual int getPerimeter() { return 1; };
    };

    class Square : public Shape {
    public:
        virtual int getPerimeter() { return 0; };
    };

    class ShapeManager {
    public:
        ShapeManager();
        void useShape() {
            if(shapes.empty())
                throw "Work stack is empty.";

            base_ptr s = shapes.front();
            s->getPerimeter();
            shapes.pop();
        }

        void submitShape(Shape &b) {
            shapes.push(base_ptr(b));
        }
    private:
        std::queue<base_ptr> shapes;
    };

    int main(int argc, char **argv) {
        ShapeManager s();
        Circle c;
        s.submitShape(c);
        s.useShape();
        return 1;
    }

It works if I declare the queue as queue<Shape*> but I don't want to deal with pointers -meaning *.

EDIT, this code compiles. Thanks everyone. This article suggested by Guillaume Racicot helps seeing clearer the situation.

#include <queue>
#include <memory>

class Shape {
public:
    virtual int getPerimeter() =0;
};

typedef std::unique_ptr<Shape> base_ptr;

class Circle : public Shape {
public:
    Circle() {};
    virtual int getPerimeter() { return 1; };
};

class Square : public Shape {
public:
    virtual int getPerimeter() { return 0; };
};

class ShapeManager {
public:
    ShapeManager();
    void useShape() {
        if(shapes.empty())
            throw "Work stack is empty.";

        base_ptr s = std::move(shapes.front());
        s->getPerimeter();
        shapes.pop();
    }

    void submitShape(base_ptr b) {
        shapes.push(std::move(b));
    }
private:
    std::queue<base_ptr> shapes;
};

int main(int argc, char **argv) {
    ShapeManager s;
    base_ptr c = std::make_unique<Circle>();
    s.submitShape(std::move(c));
    s.useShape();
    return 1;
}

Upvotes: 3

Views: 433

Answers (2)

Guillaume Racicot
Guillaume Racicot

Reputation: 41840

There are many problems in your example, not just misuse of smart pointers. First, the most obvious once is your declaration of s:

ShapeManager s();

This declares a function named s that returns a ShapeManager and takes no parameter.

Maybe you meant to declare an object of type ShapeManager?

ShapeManager s{};

// Or

ShapeManager s;

Secondly, you are misusing smart pointer. You have a queue of unique pointer. A unique pointer is a RAII wrapper around a free store allocated object. That means that it's a wrapper that is constructed with an object allocated with new. In your example, you're not doing that. You are constructing unique pointer with an object that has automatic storage.

A smart pointer that points to a automatic storage allocated object is the observer pointer: is must not own, delete or try to manage anything about that object. In fact, observer pointer is a language feature instead of a library one. It's commonly called a pointer.

This is your code with usage of observer pointers:

template<typename T>
using observer_ptr = T*;

struct ShapeManager {
    void useShape() {
        if(shapes.empty())
            throw "Work stack is empty.";

        auto s = shapes.front();

        s->getPerimeter();
        shapes.pop();
    }

    void submitShape(Shape &b) {
        shapes.push(&b);
    }

private:
    std::queue<base_ptr> shapes;
};

int main() {
    ShapeManager s;
    Circle c; // Automatic storage
    Rectangle r; // Automatic storage too.

    s.submitShape(c);
    s.submitShape(r);
    s.useShape();
}

However, you might not want to hold them using automatic storage. My guess is you want to use std::unique_ptr everywhere instead. This allow the object submitted to the shape manager to outlive it's scope. For that you'll need to allocate objects on the free store. The most common way is to use std::make_unique:

struct ShapeManager {
    void useShape() {
        if(shapes.empty())
            throw "Work stack is empty.";

        // We must create a reference,
        // Using simply auto would require copy,
        // Which is prohibited by unique pointers
        auto&& s = shapes.front();

        s->getPerimeter();
        shapes.pop();
    }

    void submitShape(base_ptr b) {
        shapes.push(std::move(b));
    }

private:
    std::queue<base_ptr> shapes;
};

int main() {
    ShapeManager s;

    // Allocated on the free store,
    // The lifetime of c and r are managed by
    // The unique pointer.
    auto c = std::make_unique<Circle>();
    auto r = std::make_unique<Rectangle>();

    s.submitShape(std::move(c));
    s.submitShape(std::move(r));
    s.useShape();
}

Upvotes: 3

Pete Becker
Pete Becker

Reputation: 76523

The container is a distraction. The problem is that unique_ptr is not copyable; if it were, it wouldn't be unique. So you probably need to add a call to std::move:

base_ptr s = std::move(shapes.front());

This means something different from what the original code might have been intended to do; it removes the object from the container. If that's not what you wanted, then std::move isn't the right answer and, probably, unique_ptr is not the right mechanism.

Upvotes: 6

Related Questions