Handling smart pointers in stl container

Question

I've a class Foo<T> which has a vector of smart pointers to Shape derived classes. I'm trying to implement an at(index) member function. Here's what I would to do intuitively:

Foo<float> myfoo;
std::unique_ptr<Shape<float>> shape_ptr = myfoo.at(i);
shape_ptr->doSomething(param1, param2, ...);

When defining the at(index) function, I'm getting a compiler error message. Note that the move constructor was defined and that the Shape base class is abstract. Below, I'm giving some code for illustration purposes.

Furthermore, I found recently on the web an example on how to overload the assignment operator using std::move. I usually follow the Copy-Swap idiom. Which of those two ways for overloading the mentioned operator makes sense for my case? Below, I'm also illustrating the function's definition.

template < typename T >
class Foo{

    public:

        Foo();
        Foo( Foo && );
        ~Foo();

        void swap(Foo<T> &);
        //Foo<T> & operator =( Foo<T> );
        Foo<T> & operator =( Foo<T> && );

        std::unique_ptr<Shape<T> > at ( int ) const; // error here!

        int size() const;

    private:

        std::vector< std::unique_ptr<Shape<T> > > m_Bank;
};

template < typename T >
Foo<T>::Foo( Foo && other)
    :m_Bank(std::move(other.m_Bank))
{

}

/*template < typename T >
void Filterbank<T>::swap(Filterbank<T> & refBank ){

    using std::swap;
    swap(m_Bank, refBank.m_Bank);
}

template < typename T >
Foo<T> & Filterbank<T>::operator =( Foo<T> bank ){

    bank.swap(*this);
    return (*this);
}*/

template < typename T >
Foo<T> & Foo<T>::operator =( Foo<T> && bank ){

    //bank.swap(*this);
    m_Bank = std::move(bank.m_Bank);
    return (*this);
}

template < typename T >
std::unique_ptr<Shape<T> > Foo<T>::at( int index ) const{
    return m_Bank[index]; // Error here! => error C2248: 'std::unique_ptr<_Ty>::unique_ptr' : cannot access private member declared in class 'std::unique_ptr<_Ty>'
}

Answer 1

Use Boost's pointer containers instead of a standard container with unique_ptr. They're designed for this kind of usage.

Answer 2

I think you should be using shared_ptr here instead.

Only one unique_ptr can own the shared resource. If you are able to do what you intend ie return a unique_ptr by value then the one in the vector will be destroyed which is probably what you don't want.

Answer 3

Q1: What to do with Foo::at( int ) const such that you can:

myfoo.at(i)->doSomething(param1, param2, ...);

without transferring ownership out of the vector<unique_ptr<Shape<T>>>.

A1: Foo::at( int ) const should return a const std::unique_ptr<Shape<T> >&:

template < typename T >
const std::unique_ptr<Shape<T> >&
Foo<T>::at( int index ) const
{
    return m_Bank[index];
}

Now your can dereference the const unique_ptr and call any member of Shape they want (const or non-const). If they accidentally try to copy the unique_ptr, (which would transfer ownership out of Foo) they will get a compile time error.

This solution is better than returning a non-const reference to unique_ptr as it catches accidental ownership transfers out of Foo. However if you want to allow ownership transfers out of Foo via at, then a non-const reference would be more appropriate.

Q2: Furthermore, I found recently on the web an example on how to overload the assignment operator using std::move. I usually follow the Copy-Swap idiom. Which of those two ways for overloading the mentioned operator makes sense for my case?

A2: I'm not sure what ~Foo() does. If it doesn't do anything, you could remove it, and then (assuming fully conforming C++11) you would automatically get correct and optimal move constructor and move assignment operator (and the proper deleted copy semantics).

If you can't remove ~Foo() (because it does something important), or if your compiler does not yet implement automatic move generation, you can supply them explicitly, as you have done in your question.

Your move constructor is spot on: Move construct the member.

Your move assignment should be similar (and is what would be automatically generated if ~Foo() is implicit): Move assign the member:

template < typename T >
Foo<T> & Foo<T>::operator =( Foo<T> && bank )
{
    m_Bank = std::move(bank.m_Bank);
    return (*this);
}

Your Foo design lends itself to being Swappable too, and that is always good to supply:

friend void swap(Foo& x, Foo& y) {x.m_Bank.swap(y.m_Bank);}

Without this explicit swap, your Foo is still Swappable using Foo's move constructor and move assignment. However this explicit swap is roughly twice as fast as the implicit one.

The above advice is all aimed at getting the very highest performance out of Foo. You can use the Copy-Swap idiom in your move assignment if you want. It will be correct and slightly slower. Though if you do be careful that you don't get infinite recursion with swap calling move assignment and move assignment calling swap! :-) Indeed, that gotcha is just another reason to cleanly (and optimally) separate swap and move assignment.

Update

Assuming Shape looks like this, here is one way to code the move constructor, move assignment, copy constructor and copy assignment operators for Foo, assuming Foo has a single data member:

std::vector< std::unique_ptr< Shape > > m_Bank;

...

Foo::Foo(Foo&& other)
    : m_Bank(std::move(other.m_Bank))
{
}

Foo::Foo(const Foo& other)
{
    for (const auto& p: other.m_Bank)
        m_Bank.push_back(std::unique_ptr< Shape >(p ? p->clone() : nullptr));
}

Foo&
Foo::operator=(Foo&& other)
{
    m_Bank = std::move(other.m_Bank);
    return (*this);
}

Foo&
Foo::operator=(const Foo& other)
{
    if (this != &other)
    {
        m_Bank.clear();
        for (const auto& p: other.m_Bank)
            m_Bank.push_back(std::unique_ptr< Shape >(p ? p->clone() : nullptr));
    }
    return (*this);
}

If your compiler supports defaulted move members, the same thing could be achieved with:

    Foo(Foo&&) = default;
    Foo& operator=(Foo&&) = default;

for the move constructor and move assignment operator.

The above ensures that at all times each Shape is owned by only one smart pointer/vector/Foo. If you would rather that multiple Foos share ownership of Shapes, then you can have as your data member:

std::vector< std::shared_ptr< Shape > > m_Bank;

And you can default all of move constructor, move assignment, copy constructor and copy assignment.

Answer 4

It seems like you should just be returning a reference here:

Shape<T> & Foo<T>::at( int index ) const{
    return *m_Bank[index];
}