C++: How to push unique_ptr to deque?

Question

In my project I need container that holds smart pointers to an data unit instances. I write the class (simple example):

template <typename T>
class Queue
{
public:

    void push(const T & param) 
    {
        m_deque.push_front(param);
    }

private:
    std::deque<T> m_deque;
};

Than I want to push the one:

int main()
{

    Queue< std::unique_ptr<DataBox> > queue;

    std::unique_ptr<DataBox> d1(new DataBox(11));

    queue.push(d1);

    return 0;
}

And VS2017 compiler says that I can't do this:

Error C2280 std::unique_ptr<DataBox,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)': attempting to reference a deleted function

As I understand the reason of the error is attempt to make copy of unique_ptr. But if I change signature to:

void push(T && param) {...}

and the function call

queue.push( std::move(d1) );

I have this error again. So the question is - how should I implement push() that will move unique_ptr to the queue?

You likely just forgot the `std::move` for `m_deque.push_front(std::move(param));` — François Andrieux, Jul 25 '18 at 17:59
Unrelated: If C++14 or more recent is available and strong exception safety is desired, [consider using `std::make_unique`](https://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique). Link to [some reasons why](https://stackoverflow.com/questions/37514509/advantages-of-using-stdmake-unique-over-new-operator) even if exception safety isn't required. — user4581301, Jul 25 '18 at 18:08

score 6 · Answer 1 · answered Jul 25 '18 at 18:29

6

You should modify your class to handle move only type:

template <typename T>
class Queue
{
public:

    void push(const T & param) 
    {
        m_deque.push_front(param);
    }

    void push(T&& param) 
    {
        m_deque.push_front(std::move(param));
    }


private:
    std::deque<T> m_deque;
};

With usage:

int main()
{
    Queue< std::unique_ptr<DataBox> > queue;

    std::unique_ptr<DataBox> d1(new DataBox(11));

    queue.push(std::move(d1));
}

answered Jul 25 '18 at 18:29

Jarod42

173,454
13
146
250

What I need `void push(const T & param) ` for? Looks like universal reference is enough. Isn't it? – Yura Jul 25 '18 at 18:33
1

@Yura That is not a universal reference. It is an rvalue reference. Universal references are only formed from rvalue references to deduced template parameters, and `T` is not deduced in this case. – Miles Budnek Jul 25 '18 at 18:37

Killzone Kid · Accepted Answer · 2018-07-25T19:33:49.297

1

If you would like to make it universal, so that push can take either l or r value you could try something like this:

#include <iostream>
#include <memory>
#include <deque>
#include <utility>

class DataBox 
{
public:
    DataBox(int i) {}
};

template <typename T>
class Queue
{
public:
    template <typename U>
    void push(U &&param)
    {
        m_deque.push_front(std::forward<T>(param));
    }

private:
    std::deque<T> m_deque;
};      

int main()
{
    Queue<std::unique_ptr<DataBox>> queue;
    std::unique_ptr<DataBox> d1 = std::make_unique<DataBox>(11);
    std::unique_ptr<DataBox> d2 = std::make_unique<DataBox>(22);
    queue.push(d1);
    queue.push(std::move(d2));

    return 0;
}

https://ideone.com/ixzEmN

edited Jul 25 '18 at 19:33

answered Jul 25 '18 at 19:24

Killzone Kid

5,839
3
12
33

Don't you mean `std::forward` ? – Jarod42 Jul 25 '18 at 22:21
normal, `std::move(d1)` should be used. currently, you move with your `std::forward` unconditionally :/ [Demo](https://ideone.com/4zm4Kz) – Jarod42 Jul 25 '18 at 22:58
@Jarod42 That is correct, the argument is always moved with this arrangement, but the argument is `unique_ptr` so I suppose this is what is wanted? – Killzone Kid Jul 25 '18 at 23:38

score 0 · Answer 3 · answered Jul 31 '18 at 22:08

You have the following options.

Option 1: concise and usually fast

template <typename T>
class Queue {
public:
    void push(T param) {
        m_deque.push_front(std::move(param));
    }

private:
    std::deque<T> m_deque;
};

I would recommend this for most practical cases, since it's simple. However, if T isn't efficiently movable, it will do two copies instead of one.

Option 2: always fast

template <typename T>
class Queue {
public:
    void push(const T& param) {
        m_deque.push_front(param);
    }
    void push(T&& param) {
        m_deque.push_front(std::move(param));
    }

private:
    std::deque<T> m_deque;
};

This is the most optimal solution, but it might be an overkill. If T is efficiently movable (and you should strive to make all types efficiently movable), then it does exactly the same amount of copies as option 1:

Queue<std::vector<int>> q;
std::vector<int> v;
q.push(v);             // one copy, both with option 1 and option 2
q.push(std::move(v));  // no copies, both with option 1 and option 2

However:

Queue<NotEfficientlyMovableType> q;
NotEfficientlyMovableType x;
q.push(x);             // one copy with option 2, two copies with option 1
q.push(std::move(x));  // same (so it doesn't really make sense)

Option 3: concise, always fast, but has caveats

template <typename T>
class Queue {
public:
    template <typename U>
    void push(U&& param)
    {
        m_deque.push_front(std::forward<T>(param));
    }

private:
    std::deque<T> m_deque;
};

With option 3 you only get to write one function and it always does the minimum number of copies (like option 2). However, it requires template argument deduction, which can break valid code. For example, this works with options 1 and 2, but not with option 3:

Queue<std::pair<int, int>> q;
q.push({1, 2});

C++: How to push unique_ptr to deque?

3 Answers3