Copy-and-Move Concept Using Smart Pointer

Question

I tried to implement a simple container class using the copy-and-move idiom. I am learning the changes to C++ as of C++11 and later.

1. Is the code exception safe?
2. I chose to use a smart pointer. Is std::unique_ptr a safe choice? I read that it should not be copied, but since I am doing the copying of the content explicitly, and since it provides a swap function, I could not see any objection. Is there a preferred practice, considering the array content type might be any type including an object type?
3. When I let the compiler do the copy for the copy assignment operator (like ...::operator=(MyContainer other)) I got an ambiguity with the move assignment operator. So I chose to put the copy operation into the function (which I prefer visually, it might look naive and oldschool, but as an aging brain I find it more explicit). Also here: Is there another, preferred practice? And to overcome my personal preference: What is the real difference of letting the compiler do the copy? Both are taking place on the stack, aren't they?

Header part

class MyContainer
{
private:
    std::size_t mSize;
    std::unique_ptr<int[]> mArray;

    static void _swap(MyContainer& first, MyContainer& second);
    void _reset(void);

public:
    MyContainer(std::size_t size); // default constructor
    MyContainer(const MyContainer& other); // copy constructor
    MyContainer(MyContainer&& other) noexcept; // move constructor
    MyContainer& operator=(const MyContainer& other); // copy assignment operator
    MyContainer& operator=(MyContainer&& other) noexcept; // move assignment operator
    void dump(void);
    /* ... */
    ~MyContainer(void);
};

Source part

void MyContainer::_swap(MyContainer& first, MyContainer& second) // static
{
    std::swap(first.mSize, second.mSize);
    first.mArray.swap(second.mArray);
}

void MyContainer::_reset(void)
{
    mSize = 0;
    mArray.reset();
}

MyContainer::MyContainer(std::size_t size = 0) // default constructor
    : mSize(size),
    mArray(mSize ? new int[mSize] : nullptr)
{
    // for testing purpose only
    for (int i = 0; i < mSize; i++)
        mArray.get()[i] = i;
}

MyContainer::MyContainer(const MyContainer& other) // copy constructor
    : mSize(other.mSize),
    mArray(mSize ? new int[mSize] : nullptr)
{
    std::copy(other.mArray.get(), other.mArray.get() + mSize, mArray.get());
}

MyContainer::MyContainer(MyContainer&& other) noexcept // move constructor
    : MyContainer() // init using default constructor (C++11)
{
    _swap(*this, other);
}

MyContainer& MyContainer::operator=(const MyContainer& other) // copy assignment
{
    MyContainer tmp(other);
    _swap(*this, tmp);
    return *this;
}

MyContainer& MyContainer::operator=(MyContainer&& other) noexcept // move assignment
{
    if (this != &other)
    {
        _reset();
        _swap(*this, other);
    }
    return *this;
}


void MyContainer::dump(void)
{
    std::cout << "Size: " << mSize << std::endl;
    for (int i = 0; i < mSize; i++)
        std::cout << mArray.get()[i] << std::endl;
}

MyContainer::~MyContainer(void)
{
}

Answer 1

unique_ptr is fine here. It's used where you would have used a plain pointer before.

But don't use naked new — use make_unique. Oh, and I think a unique_ptr of array types was not in C++11 but needs C++14.

Don’t use (void) for () in parameter lists! That was never proper, even in c++98. (it was supported to allow compatibility with C headers, but in Bjarne’s own words it’s “an abomination”.

Don’t write an empty destructor. Use =default to emphasize that it exists, or just leave it off completely.

Why have a private _swap static function instead of a non-member overload of swap that anyone can use?

Another important new feature you have not embraced yet is the range for loop. Use that instead of the lagacy for loops you have that go over the whole array.

Answer 2

Your specific questions

Yes, I'd say that unique_ptr is exactly right. The only place we use new, there's no window for any exception to leak the memory. The destructor cleans up for us, and we never use the pointed-to memory after it's gone.

Well done on a robust implementation!

Consider the following to be polishing, rather than ripping up your code.

Missing headers

The header file needs

#include <cstddef>              // size_t
#include <memory>               // unique_ptr

And the implementation needs:

#include <algorithm>
#include <iostream>

Default constructor

The default argument should be supplied in the header, not the implementation. It's a good idea to make all one-argument constructors (other than copy/move) explicit, unless we really want them to be converting constructors.

If we include <numeric>, we could use std::iota() instead of the loop to generate the dummy data (I don't think that's necessarily better, but you should know it exists). That's helped if we provide begin() and end() methods:

public:
    int *begin() { return mArray.get(); }
    int *end() { return begin() + mSize; }
    int const *begin() const { return mArray.get(); }
    int const *end() const { return begin() + mSize; }

MyContainer::MyContainer(std::size_t size)
    : mSize(size),
      mArray(mSize ? new int[mSize] : nullptr)
{
    // for testing purpose only
    std::iota(begin(), end(), 0);
}

Copy constructor

This looks good, apart from the missing check to avoid passing a null pointer to std::copy; it can also be simplified using the begin() and end() methods:

MyContainer::MyContainer(const MyContainer& other)
    : mSize(other.mSize),
      mArray(mSize ? new int[mSize] : nullptr)
{
    if (mArray) {
        std::copy(other.begin(), other.end(), begin());
    }
}

Assignment operators

These are slightly easier to implement if we write a swap() as a member, rather than as a class method:

MyContainer& MyContainer::swap(MyContainer& other)
{
    using std::swap;
    // Note: no need to check other != this, as self-swap is safe
    swap(mSize, other.mSize);
    swap(mArray, other.mArray);
    return *this;
}

It can be a public method, just like the standard containers.

Then the assignments are simply

MyContainer& MyContainer::operator=(const MyContainer& other)
{
    MyContainer tmp(other);
    return swap(tmp);
}

MyContainer& MyContainer::operator=(MyContainer&& other) noexcept
{
    return swap(other);
}

However, we can combine this into a single method if we pass-by-value - at no cost in the number of memory allocations:

MyContainer& MyContainer::operator=(MyContainer other) noexcept
{
    return swap(other);
}

Although this method is noexcept remember that if the compiler needs to copy into other, that copy may throw - we maintain strong exception safety even then.

Note also that there's no need to reset before swapping in the new value - we're leaving other in a valid state, ready for its destructor.

Destructor

Don't declare an empty destructor. If you create a class to be used as a base class, write

virtual ~MyContainer() = default;

But in this case, it's better to omit the destructor completely.

Reset method

I don't think this is needed - we can simply assign an empty MyCollection.

Dump method

Prefer to pass std::ostream to use, rather than only supporting output to std::cout.

Avoid std::endl unless you have a real need to flush the stream.

Use an unsigned type for the loop variable (or, use iterators).

---

Modified code

This has no leaks (tested in Valgrind). I believe it's still exception-safe, though haven't tested that specifically.

// Declaration

#include <cstddef>
#include <iosfwd>
#include <memory>

class MyContainer
{
private:
    std::size_t mSize;
    std::unique_ptr<int[]> mArray;

public:
    explicit MyContainer(std::size_t size = 0);
    MyContainer(const MyContainer& other);
    MyContainer(MyContainer&& other) noexcept;
    MyContainer& operator=(MyContainer other) noexcept;
    MyContainer& swap(MyContainer& other);
    void dump(std::ostream& os);

    int *begin() { return mArray.get(); }
    int *end() { return begin() + mSize; }
    int const *begin() const { return mArray.get(); }
    int const *end() const { return begin() + mSize; }
};

// Implementation

#include <algorithm>
#include <ostream>
#include <numeric>

MyContainer::MyContainer(std::size_t size)
    : mSize(size),
      mArray(mSize ? new int[mSize] : nullptr)
{
    // canned data for tests - remove for real use
    if (mArray) {
        std::iota(begin(), end(), 0);
    }
}

MyContainer::MyContainer(const MyContainer& other)
    : mSize(other.mSize),
      mArray(mSize ? new int[mSize] : nullptr)
{
    if (mArray) {
        std::copy(other.begin(), other.end(), begin());
    }
}

MyContainer::MyContainer(MyContainer&& other) noexcept
    : mSize{0},
      mArray{nullptr}
{
    swap(other);
}

MyContainer& MyContainer::operator=(MyContainer other) noexcept
{
    return swap(other);
}

MyContainer& MyContainer::swap(MyContainer& other)
{
    // Note: no need to check other != this, as self-swap is safe
    std::swap(mSize, other.mSize);
    mArray.swap(other.mArray);
    return *this;
}


void MyContainer::dump(std::ostream& os)
{
    os << "Size: " << mSize << '\n';
    for (auto it = begin();  it != end();  ++it)
        os << ' ' << *it;
    os << '\n';
}

// Test

#include <iostream>
int main()
{
    MyContainer a{6};
    auto b = a;
    a.dump(std::clog);
    b.dump(std::clog);
    std::clog << '\n';

    MyContainer c;
    c = std::move(a);
    a.dump(std::clog);
    c.dump(std::clog);
}

---

Suggested extensions

• Add a public size() and empty() methods - once we have size(), then dump() can be a public non-member function.
• Add a constructor that takes std::initializer_list<int>, and one that takes a pair of iterators (the former might delegate to the latter; use std::distance() to obtain the required size from iterators).
• Make it generic (i.e. a template class)
• Add the rest of the begin/end methods (cbegin(), cend(), crbegin(), crend(), and both const and non-const implementations of rbegin() and rend()). Declare the usual names for the iterator types: iterator, const_iterator, reverse_iterator, const_reverse_iterator.