Questions about the move assignment operator

Question

Imagine the following class that manages a resource (my question is only about the move assignment operator):

struct A
{
    std::size_t s;
    int* p;
    A(std::size_t s) : s(s), p(new int[s]){}
    ~A(){delete [] p;}
    A(A const& other) : s(other.s), p(new int[other.s])
    {std::copy(other.p, other.p + s, this->p);}
    A(A&& other) : s(other.s), p(other.p)
    {other.s = 0; other.p = nullptr;}
    A& operator=(A const& other)
    {A temp = other; std::swap(*this, temp); return *this;}
    // Move assignment operator #1
    A& operator=(A&& other)
    {
        std::swap(this->s, other.s);
        std::swap(this->p, other.p);
        return *this;
    }
    // Move assignment operator #2
    A& operator=(A&& other)
    {
        delete [] p;
        s = other.s;
        p = other.p;
        other.s = 0;
        other.p = nullptr;
        return *this;
     } 
};

Question:

What are the advantages and disadvantages of the two move assignment operators #1 and #2 above? I believe the only difference I can see is that std::swap preserves the storage of the lhs, however, I don't see how that would be useful as rvalues would be destroyed anyways. Maybe the only time would be with something like a1 = std::move(a2);, but even in this case I don't see any reason to use #1.

Answer 1

The assignment operator posted by DeadMG is doing all the right thing if swap()ing involved objects can't throw. Unfortunately, this can't be always guaranteed! In particular, if you have stateful allocators and this won't work. If the allocators can differ it seems you want separate copy and move assignment: the copy constructor would unconditionally create a copy passing in the allocator:

T& T::operator=(T const& other) {
    T(other, this->get_allocator()).swap(*this);
    return * this;
}

The move assignment would test if the allocators are identical and, if so, just swap() the two objects and otherwise just call the copy assignment:

T& operator= (T&& other) {
    if (this->get_allocator() == other.get_allocator()) {
        this->swap(other);
    }
    else {
        *this = other;
    }
    return *this;
}

The version taking a value is a much simpler alternative which should be preferred if the noexcept(v.swap(*this)) is true.

This implicitly also answers the original qurstion: in the presence of throwing swap() and move assignment both implementations are wrong as they aren't basic exception safe. Assuming the only source of exceptions in swap() are mismatched allocators the implementation above is strong exception safe.

Answer 2

It's more valid to use #1 than #2 because if you use #2, you're violating DRY and duplicating your destructor logic. Secondly, consider the following assignment operator:

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

This is both copy and move assignment operators for no duplicated code- an excellent form.

Answer 3

This is a case where you should really measure.

And I'm looking at the OP's copy assignment operator and seeing inefficiency:

A& operator=(A const& other)
    {A temp = other; std::swap(*this, temp); return *this;}

What if *this and other have the same s?

It seems to me that a smarter copy assignment could avoid making a trip to the heap if s == other.s. All it would have to do is the copy:

A& operator=(A const& other)
{
    if (this != &other)
    {
        if (s != other.s)
        {
            delete [] p;
            p = nullptr;
            s = 0;
            p = new int[other.s];
            s = other.s;
        }
        std::copy(other.p, other.p + s, this->p);
    }
    return *this;
}

If you don't need strong exception safety, only basic exception safety on copy assignment (like std::string, std::vector, etc.), then there is a potential performance improvement with the above. How much? Measure.

I've coded this class three ways:

Design 1:

Use the above copy assignment operator and the OP's move assignment operator #1.

Design 2:

Use the above copy assignment operator and the OP's move assignment operator #2.

Design 3:

DeadMG's copy assignment operator for both copy and move assignment.

Here is the code I used to test:

#include <cstddef>
#include <algorithm>
#include <chrono>
#include <iostream>

struct A
{
    std::size_t s;
    int* p;
    A(std::size_t s) : s(s), p(new int[s]){}
    ~A(){delete [] p;}
    A(A const& other) : s(other.s), p(new int[other.s])
    {std::copy(other.p, other.p + s, this->p);}
    A(A&& other) : s(other.s), p(other.p)
    {other.s = 0; other.p = nullptr;}
    void swap(A& other)
    {std::swap(s, other.s); std::swap(p, other.p);}
#if DESIGN != 3
    A& operator=(A const& other)
    {
        if (this != &other)
        {
            if (s != other.s)
            {
                delete [] p;
                p = nullptr;
                s = 0;
                p = new int[other.s];
                s = other.s;
            }
            std::copy(other.p, other.p + s, this->p);
        }
        return *this;
    }
#endif
#if DESIGN == 1
    // Move assignment operator #1
    A& operator=(A&& other)
    {
        swap(other);
        return *this;
    }
#elif DESIGN == 2
    // Move assignment operator #2
    A& operator=(A&& other)
    {
        delete [] p;
        s = other.s;
        p = other.p;
        other.s = 0;
        other.p = nullptr;
        return *this;
     } 
#elif DESIGN == 3
    A& operator=(A other)
    {
        swap(other);
        return *this;
    }
#endif
};

int main()
{
    typedef std::chrono::high_resolution_clock Clock;
    typedef std::chrono::duration<float, std::nano> NS;
    A a1(10);
    A a2(10);
    auto t0 = Clock::now();
    a2 = a1;
    auto t1 = Clock::now();
    std::cout << "copy takes " << NS(t1-t0).count() << "ns\n";
    t0 = Clock::now();
    a2 = std::move(a1);
    t1 = Clock::now();
    std::cout << "move takes " << NS(t1-t0).count() << "ns\n";
}

Here is the output I got:

$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=1  test.cpp 
$ a.out
copy takes 55ns
move takes 44ns
$ a.out
copy takes 56ns
move takes 24ns
$ a.out
copy takes 53ns
move takes 25ns
$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=2  test.cpp 
$ a.out
copy takes 74ns
move takes 538ns
$ a.out
copy takes 59ns
move takes 491ns
$ a.out
copy takes 61ns
move takes 510ns
$ clang++ -std=c++11 -stdlib=libc++ -O3 -DDESIGN=3  test.cpp 
$ a.out
copy takes 666ns
move takes 304ns
$ a.out
copy takes 603ns
move takes 446ns
$ a.out
copy takes 619ns
move takes 317ns

DESIGN 1 looks pretty good to me.

Caveat: If the class has resources that need to be deallocated "quickly", such as mutex lock ownership or file open-state ownership, the design-2 move assignment operator could be better from a correctness point of view. But when the resource is simply memory, it is often advantageous to delay deallocating it as long as possible (as in the OP's use case).

Caveat 2: If you have other use cases you know to be important, measure them. You might come to different conclusions than I have here.

Note: I value performance over "DRY". All of the code here is going to be encapsulated within one class (struct A). Make struct A as good as you can. And if you do a sufficiently high quality job, then your clients of struct A (which may be yourself) won't be tempted to "RIA" (Reinvent It Again). I much prefer to repeat a little code within one class, rather than repeat the implementation of entire classes over and over again.