The efficient way to write move copy and move assignment constructors

Question

Are the following assignment and copy move constructors the most efficient? if anybody have other way please tell me? I mean what bout std::swap? and calling assignment through copy constructor is safe in the code below?

#include <iostream>
#include <functional>
#include <algorithm>
#include <utility>

using std::cout;
using std::cin;
using std::endl;
using std::bind;


class Widget
{

public:

    Widget(int length)
        :length_(length),
        data_(new int[length])
    {
        cout<<__FUNCTION__<<"("<<length<<")"<<endl;
    }

    ~Widget()
    {
        cout<<endl<<__FUNCTION__<<"()"<<endl;
        if (data_)
        {
            cout<<"deleting source"<<endl;
        } 
        else
        {
            cout<<"deleting Moved object"<<endl;
        }

        cout<<endl<<endl;
    }

    Widget(const Widget& other)
        :length_(other.length_),
        data_(new int[length_])
    {
        cout<<__FUNCTION__<<"(const Widget& other)"<<endl;
        std::copy(other.data_,other.data_ + length_,data_);
    }

    Widget(Widget&& other)
/*
        :length_(other.length_),
        data_(new int[length_])*/
    {
        cout<<__FUNCTION__<<"(Widget&& other)"<<endl;
        length_ = 0;
        data_ = nullptr;
        std::swap(length_,other.length_);
        std::swap(data_,other.data_);
    }

    Widget& operator = (Widget&& other)
    {
        cout<<__FUNCTION__<<"(Widget&& other)"<<endl;

        std::swap(length_,other.length_);
        std::swap(data_,other.data_);

        return *this;
    }

    Widget& operator = (const Widget& other)
    {
        cout<<__FUNCTION__<<"(const Widget& other)"<<endl;
        Widget tem(other);
        std::swap(length_,tem.length_);
        std::swap(data_,tem.data_);

        return *this;
    }
    int length()
    {
        return length_;
    }

private:

    int length_;
    int* data_;
};



int main()
{
    {
        Widget w1(1);
        Widget w2(std::move(Widget(2)));

        w1 = std::move(w2);
    }


    cout<<"ENTER"<<endl;
    cin.get();
    return 0;
}

Answer 1

Looks fine from an efficiency POV, but contains an awful lot of duplicated code. I'd

• Implement a swap() operator for your class.
• Initialize length_ and data_ where they are declared.
• Implement operations in terms of other operations whereever possible.

You might want to use std::memcpy instead of std::copy since you're dealing with a raw array anyway. Some compilers will do that for you, but probably not all of them...

Here's a de-duplicated version of your code. Note how there is only one place which needs to know how two instances of Widget are swapped. And only one place which knows how to allocate a Widget of a given size.

Edit: You usually also want to use argument-dependent lookup to locate swap, just in case you ever have non-primitive members.

Edit: Integrated @Philipp's suggestion of making the assignment operator take it's argument by value. That way, it acts as both move assignment and copy assignment operator. In the move case, not that if you pass a temporary, it won't be copied, since the move constructor, not the copy constructor will be used to pass the argument.

Edit: C++11 allows non-cost members to be called on rvalues for compatibility with previous versions of the standard. This allows weird code like Widget(...) = someWidget to compile. Making operator= require an lvalue for this by putting & after the declaration prevents that. Note though that the code is correct even without that restriction, but it nevertheless seems like a good idea, so I added it.

Edit: As Guillaume Papin pointed out, the destructor should use delete[] instead of plain delete. The C++ standard mandates that memory allocated via new [] be deleted via delete [], i.e. it allows new' andnew []` to use different heaps.

class Widget
{
public:
    Widget(int length)
        :length_(length)
        ,data_(new int[length])
    {}

    ~Widget()
    {
        delete[] data_;
    }

    Widget(const Widget& other)
        :Widget(other.length_)
    {
        std::copy(other.data_, other.data_ + length_, data_);
    }

    Widget(Widget&& other)
    {
        swap(*this, other);
    }

    Widget& operator= (Widget other) &
    {
        swap(*this, other);
        return *this;
    }

    int length() const
    {
        return length_;
    }

private:
    friend void swap(Widget& a, Widget& b);

    int length_ = 0;
    int* data_ = nullptr;
};

void swap(Widget& a, Widget& b) {
    using std::swap;
    swap(a.length_, b.length_);
    swap(a.data_, b.data_);
}

Answer 2

The answer is in response to @Abdulrhman's complaint in the comments above that things fail for some (obscure) sequences of assignments. Put into a seperate answer because it's more readable that way.

The complaint was that

Widget w(2);
w = Widget(1) = std::move(w);

crashes. Here's the output I get from

Widget w(2);
w.data()[0] = 0xDEAD; w.data()[1] = 0xBEEF;
w = Widget(1) = std::move(w);
std::cerr << std::hex << w.data()[0] << w.data()[1] << std::endl;

with some code added to Widget to log constructor, destructor and assignment operator calls. Interleaves are comments about where those calls come from

w is constructed
0x7fff619c36c0: [constructor] allocated 2@0x1043dff80
temporary Widget(1) is constructed
0x7fff619c37c0: [constructor] allocated 1@0x1043e0180
first (right) assignment operator argument is constructed. w is empty afterwards!
0x7fff619c3800: [default constructor] empty
0x7fff619c3800: [move constructor] stealing 2@0x1043dff80 from 0x7fff619c36c0, replacing with 0@0x0
first assignment operator does it's job, i.e. moves from by-value argument.
0x7fff619c37c0: [assignment] stealing 2@0x1043dff80 from 0x7fff619c3800, replacing with 1@0x1043e0180
second (left) assignment operator arguments is constructed
0x7fff619c3780: [constructor] allocated 2@0x1043e0280
0x7fff619c3780: [copy constructor] copying 2@0x1043dff80 from 0x7fff619c37c0
second assignment operator does it's job, i.e. moves from by-value argument
0x7fff619c36c0: [assignment] stealing 2@0x1043e0280 from 0x7fff619c3780, replacing with 0@0x0
second assingment operator's by-value argument is destructed
0x7fff619c3780: [destructor] deleting 0@0x0
first assignment operator's by-value argument is destructed
0x7fff619c3800: [destructor] deleting 1@0x1043e0180
temporary created as Widget(1) is destructed.
0x7fff619c37c0: [destructor] deleting 2@0x1043dff80
data contains in "w" after assignments.
deadbeef
finally, "w" is destructed.
0x7fff619c36c0: [destructor] deleting 2@0x1043e0280

I can see no problem there, and compiling this with clang and -faddress-sanitizer, -fcatch-undefined-behaviour doesn't complain either.

Note, though, that the second assigment (the left = operator) copies instead of moving. This is because the first (right) assignment operator returns an lvalue-reference.

Answer 3

You don't need so many swaps and assignments in your move constructor. This:

Widget(Widget&& other) :
    length( other.length_ ), data( other.data_ )
{
    other.length_ = 0;
    other.data_ = nullptr;
}

does the minimum work for the move constructor: 4 assignments in total. Your version had 8, counting the ones in the calls to swap().

Your move assignment is OK, but you might want to consider just writing one operator=() to cover both cases:

Widget &operator=( Widget other ) {
    delete data_;
    data_ = other.data_;
    other.data_ = nullptr;
    length_ = other.length_;
    other.length_ = 0;
    return *this;
}

This is slightly less efficient than your version, in that it can move twice.