Is this a proper implementation of the Rule of Five (or Rule of Four and 1/2)?

Question

I am studying the Rule of Five and it's cousins (Rule of Four and 1/2, Copy and Swap idiom, Friend Swap Function).

I implemented the Rule of Four and 1/2 on a test class. It compiles well. Is there any hidden mistake in my implementation?

I am particulary preoccupied about the unique_ptrs stored in the m_unorederd_map property that I moved in the copy constructor as they can't be copied. Is this the proper way to deal with unique_ptrs in classes?

someclass.h

#ifndef SOMECLASS_H
#define SOMECLASS_H

#include "someotherclass.h"

#include <QString>
#include <QStringList>

#include <memory>
#include <string>
#include <unordered_map>

class SomeClass
{
    QString m_qstring;
    std::unordered_map<std::string, std::unique_ptr<SomeOtherClass>> m_unordered_map;
    int m_int;
    std::string m_string;
    QStringList m_qstringlist;

public:
    SomeClass() = default;

    // Rule of 5 (or Rule of 4 and 1/2)
    // From https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom#3279550
    ~SomeClass() = default;                               // Destructor
    SomeClass(SomeClass &other);                          // Copy constructor
    SomeClass(SomeClass &&other);                         // Move constructor
    SomeClass &operator=(SomeClass other);                // Copy/Move assignment operator
    friend void swap(SomeClass &first, SomeClass &second) // Friend swap function
    {
        using std::swap;
        first.m_qstring.swap(second.m_qstring);
        first.m_unordered_map.swap(second.m_unordered_map);
        swap(first.m_int, second.m_int);
        swap(first.m_string, second.m_string);
        first.m_qstringlist.swap(second.m_qstringlist);
    }
};

#endif // SOMECLASS_H

someclass.cpp

#include "someclass.h"

// Copy constructor
SomeClass::SomeClass(SomeClass &other)
    : m_qstring(other.m_qstring),
      m_int(other.m_int),
      m_string(other.m_string),
      m_qstringlist(other.m_qstringlist)
{
    // m_unordered_map holds unique_ptrs which can't be copied.
    // So we move it.
    m_unordered_map = std::move(other.m_unordered_map);
}

// Move constructor
SomeClass::SomeClass(SomeClass &&other)
    : SomeClass()
{
    swap(*this, other);
}

// Copy/Move assignment operator
SomeClass &SomeClass::operator=(SomeClass other)
{
    swap(*this, other);
    return *this;
}

Answer 1

Most important of all:

• This class doesn't need custom copy/move operations nor the destructor, so rule of 0 should be followed.

Other things:

• I don't like swap(*this, other); in the move ctor. It forces members to be default-constructed and then assigned. A better alternative would be to use a member initializer list, with std::exchange.

  If initializing all members gets tedious, wrap them in a structure. It makes writing the swap easier too.

• Copy constructor must take the parameter by a const reference.

• unique_ptrs which can't be copied. So we move it. is a bad rationale. If your members can't be copied, don't define copy operations. In presence of custom move operations, the copy operations will not be generated automatically

• Move operations (including the by-value assignment) should be noexcept, because standard containers won't use them otherwise in some scenarios.

• SomeClass() = default; causes members that are normally uninitialized (int m_int;) to sometimes be zeroed, depending on how the class is constructed. (E.g. SomeClass x{}; zeroes it, but SomeClass x; doesn't.)

  Unless you want this behavior, the constructor should be replaced with SomeClass() {}, and m_int should probably be zeroed (in class body).