Implementing swap/copy functions: is there a better way?

Question

These are how I implemented the swap and copy functions for my queue.

Is there a better way?

template <class T>
void Queue<T>::copy(Queue<T> const & other)
{
    if(this == &other) 
        return;

    if(m_size != 0)
        this->clear();

    this->m_cap = other.m_cap;
    this->enqueue(other);
}

template <class T>
void Queue<T>::swap(Queue<T> const & other)
{
    if(this == &other)
        return;

    std::swap(this->m_front, other.m_front);
    std::swap(this->m_back, other.m_back);
    std::swap(this->m_size, other.m_size);
    std::swap(this->m_cap, other.m_cap);
}

Thank you:)

Why do you have a method named `copy` instead of just using copy constructors and copy assignment operators? — Mooing Duck, Apr 02 '19 at 20:00
`if(this == &other)` is a sign that your copy method probably isn't exception safe. — Mooing Duck, Apr 02 '19 at 20:00
Usually a copy is accomplished with a copy constructor. And then, assignment is accomplished with a copy and swap. — jxh, Apr 02 '19 at 20:02
Did you really intend swap to take a reference to a constant? — jxh, Apr 02 '19 at 20:03
First of all "better" is subjective, second we do not have enough information even to speculate. — Slava, Apr 02 '19 at 20:16
https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom — jxh, Apr 02 '19 at 20:53

jxh · Accepted Answer · 2019-04-03T00:09:40.510

Instead of a copy method, you should instead implement a copy constructor. It would be strange for the copy constructor to be passed an instance of itself. If you insist on checking, you can use an assertion.

template <class T>
Queue<T>::Queue(Queue<T> const & other)
    : m_cap(other.m_cap), m_size(0), m_front(), m_back()
{
    assert(&other != this);
    enqueue(other);
}

Your copy method is actually an assignment. It is more natural to implement an assignment operator. This can be accomplished by following the copy-swap idiom.

template <class T>
Queue<T> & Queue<T>::operator = (Queue<T> other)
{
    swap(*this, other);
    return *this;
}

There is also an idiomatic way to implement swap (taught to me by Mooing Duck a long time ago):

template <class T>
class Queue {
//...
    friend void swap(Queue &a, Queue &b) {
        using std::swap;
        swap(a.m_front, b.m_front);
        swap(a.m_back, b.m_back);
        swap(a.m_size, b.m_size);
        swap(a.m_cap, b.m_cap);
    }
};

This way, you can use argument dependent lookup (ADL) to pick the type specific swap implementation if available. And now Queue itself has such an implementation, which is used by the assignment operator. But it can also be used in the case Queue is put inside an object that wants to implement swap for itself.

Hey jxh, thanks a lot for your answer. I found it helpful and enlightening. — Rafael Vergnaud, Apr 02 '19 at 23:51
If I may ask one question: why would we want to to use swap when overloading =? May we not want “other” to remain as it is? — Rafael Vergnaud, Apr 02 '19 at 23:59
Assignment operator takes its parameter by value not by reference. Using swap allows the the contents to be destructed when the function returns. — jxh, Apr 03 '19 at 00:00

Implementing swap/copy functions: is there a better way?

1 Answers1