Applying the copy-swap idiom to humans and employees

I came across this post which explains the copy and swap idiom. However I could not find this idiom applied to classes that have base classes. Following is my attempt of a copy-swap idiom applied to class constructors and assignment operators. In the following code class employee inherits from class human - The constructors and assignment operators of both the classes are quite similar in manner. I am more interested in making this a generic class. If you have solid suggestions about exception safety please let me know. I would like to keep the code to a minimum to serve as a guiding principal for further classes. I also understand that using a char is not recommended for a C++ project however most of the projects that I have come across use a char here and there.

```
#include
#include

class details
{
std::string optional_info;
};

class human
{
public:
char *blood_type=nullptr;
std::string dob = "";
details* di = nullptr;

//This is the swap method
void swap(human& lhs , human& rhs)
{
std::swap(lhs.blood_type , rhs.blood_type);
std::swap(lhs.dob , rhs.dob);
std::swap(lhs.di , rhs.di);
}

public:

//Regular constructor
human() {/.../}

human(const human& f)
{
//Copy the blood_type char pointer
if(f.blood_type != nullptr) {
blood_type = new char[std::strlen(f.blood_type) + 1];
strcpy(blood_type, f.blood_type);
}

//Copy the string
this->dob = f.dob;

//Copy the object detail
this->di = new details(*(f.di));
}

//Assignment operator
human& operator=(human& f)
{
if(this != &f) //Make sure they are not the same
{
human temp = f; //Call the copy constructor
swap(*this,temp); //Swap the values;
}
return *this;
}

//Move copy constructor
human(human&& f)
{
swap(*this , f);
}

//Move copy constructor
human& operator=(human&& f)
{
swap(*this , f);
return *this;
}

However I could not find this idiom applied to classes that have base classes.

TL;DR: Don't mix value semantics with polymorphism. You can't sensibly swap() two humans, because you can't sensibly swap an employee with a non-employee (even though both are humans). I say a lot more about this, far below, after we get through the basic copy-and-swap stuff.

• Does this class [human] comply with the copy-swap idiom? Is anything missing or overlooked?

Your copy constructor for human is exception-unsafe.
Here are the members that need copying:

char *blood_type = nullptr;
std::string dob = "";
details *di = nullptr;

And here is your implementation, for ease of reference. I've adjusted the whitespace for readability, and removed redundant comments, but not changed any of the non-whitespace characters:

human(const human& f)
  {
     if (f.blood_type != nullptr) {
         blood_type =  new char[std::strlen(f.blood_type) + 1];
         strcpy(blood_type, f.blood_type);
     }

     this->dob = f.dob;  // may throw std::bad_alloc [1]

     this->di = new details(*(f.di));  // may throw std::bad_alloc [2]
  }

The explicit call to operator new on line [2] might throw, in which case you'd leak this->blood_type. Or, the implicit allocations inside std::string::operator= on line [1] and std::string::string(const string&) on line [2] might throw, in which case you'd leak this->blood_type and/or this->di.

In practice, you would use std::unique_ptr and/or std::string to manage all your memory, and then you could rely on the Rule of Zero. But let's say you can't do that for some reason. Then a correct implementation of your copy constructor would look like this:

human(const human& f) : dob(f.dob)
{
    try {
        if (f.blood_type != nullptr) {
            blood_type = new char[std::strlen(f.blood_type) + 1];
            strcpy(blood_type, f.blood_type);
        }
        di = new details(*f.di);
    } catch (...) {
        delete [] blood_type;
        delete di;
        throw;
    }
}

Your copy-assignment operator is almost correct, but you're missing a const on the argument.

human& operator=(const human& f) {
     if (this != &f) {
        human temp = f;
        swap(*this, temp);
     }
     return *this;
  }

Your move constructor and move-assignment operator (not "move copy constructor"; there's no such thing) implemented as "move by swapping" are both correct.

Your swap function itself, however, is confused. You've made it a non-static member function, called as alice.swap(bob, charlie). You should instead make it a friend free function:

friend void swap(human& lhs, human& rhs) {
    using std::swap;
    swap(lhs.blood_type, rhs.blood_type);
    swap(lhs.dob, rhs.dob);  // [3]
    swap(lhs.di, rhs.di);
}

Notice that I'm using the using std::swap ADL idiom, which means that the swap invoked on line [3] is swap(string&, string&) and not swap. If you screw up and accidentally write std::swap(lhs.dob, rhs.dob), you will invoke undefined behavior in the case that &lhs == &rhs, because you'll be attempting to move a std::string into itself. (N4296 contains contradictory wording on the subject, but as of Lenexa it has been amended to clarify that indeed std::string does not support self-move.) If you're at all unsure whether swap(foo, foo) does the right thing for some member foo, please do wrap your whole swap routine in an extra layer of if (&lhs != &rhs).

Lastly, your destructor should be written as

~human() {
    delete [] blood_type;  // use the correct operator delete[]
    delete di;
}

There's no need to test for nullptr first; the built-in operator delete and operator delete[] are guaranteed to do the right thing (i.e. nothing) when you pass in nullptr.

• I noticed that the assignment operator in the above class [i.e. employee] could use improvements.

Yes; IMHO it's really sketchy to depart from the Rule of Zero in a child class. Make the new members

std::string duty = "";
std::string title = "";
details detail = {};

and the whole problem goes away. But again, for pedagogical purposes, let's assume you can't do that.

void swap(employee& lhs, employee& rhs)
{
    std::swap(lhs.duty, rhs.duty);
    std::swap(lhs.title, rhs.title);
    std::swap(lhs.detail, rhs.detail);
}

Again this should be a friend free function, not a non-static member function. However, it's got problems:

employee alice, bob;
alice.dob = "1/1/1990"; bob.dob = "4/2/1942";
swap(alice, bob);
assert(alice.dob == "1/1/1990");  // oops!

swap doesn't swap the parts of the employee that make them human!
But then if we change swap to also swap the human part, we find that we've broken your assignment operator...

employee(employee&& f)
    : human(std::move(f))   // swaps the human parts
{
    swap(*this, f);         // swaps the human parts back!
}

The right fix here is to apply

StackExchange Code Review Q#85539, answer score: 5