Applying the copy-swap idiom to humans and employees

Question

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 <iostream>
#include <cstring>

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;
  }

  ~human()
  {
     if(blood_type!=nullptr)
        delete blood_type;
     if(di!=nullptr)
         delete di;
  }
};



class employee : public human
{
  public:
  char *duty=nullptr;
  std::string title="";
  details* detail=nullptr;;

  //This si the swap method
  void swap(employee& lhs , employee& rhs)
  {
    std::swap(lhs.duty , rhs.duty);
    std::swap(lhs.title , rhs.title);
    std::swap(lhs.detail , rhs.detail);
  }


  public:
  //Regular constructor
  employee() {/*...*/}

  //Copy constructor
  //Since we provided a custom copy constructor we should also
  //call the copy constructor of the base class using initialization list
  //If we do not do that the regular constructor of base class will be called.
  employee(const employee& f) : human(f) //This will call the copy constructor of base class
  {
     //Copy the address char pointer
     if(f.duty != nullptr) {
     duty =  new char[std::strlen(f.duty) + 1];
     strcpy(duty, f.duty);
     }

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

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

  //We need to copy the base class too.
  //Assignment operator
  employee& operator=(employee& f)
  {
     if(this != &f)               //Make sure they are not the same
     {
        employee temp = f;             //Call the copy constructor
        human::operator=(temp); //<---Design question regarding this at end.
        swap(*this,temp);         //Swap the values;
     }
     return *this;
  }

  //Move copy constructor
  employee(employee&& f) : human( std::move( f))    //call move constructor of base class
  {
    //Important
    //Now f is an lvalue (since it has a name) that is holding an rvalue.
    swap(*this , f);
  }

  //Move copy constructor
  //Rvalue reference to hold temporaries as parameter
  employee& operator=(employee&& f)
  {
    human::operator=(std::move(f)); //Call assignment operator of base classa
    swap(*this , f);
    return *this;
  }

  ~employee()
  {
     if(duty!=nullptr)
        delete duty;
     if(detail!=nullptr)
         delete detail;
  }
};

Questions:

1. Does this class comply with the copy-swap idiom? Is anything missing or overlooked?

2. I noticed that the assignment operator in the above class could use improvements. For instance, first it creates a deep copy of the object using its copy constructor (during which a copy of the base class is made) after that the following call is made:

   human::operator=(temp) ;
   

   This statement calls the copy assignment operator of the base class which in return makes another deep copy using its own copy constructor. I wanted to know if there was a better way of accomplishing this.

Answer 1

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.

1. 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<string>. 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.

2. 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 the "move by swapping" idiom to your employee move constructor as well. You don't need to keep mentioning the fact that an employee is a human.

employee(employee&& f)  // default-construct all the parts
{
    swap(*this, f);         // swap all the parts at once
}

employee& operator=(employee&& f)
{
    swap(*this, f);
    return *this;
}

So then how do we write swap correctly for an employee? Well, that gets us back to the problem that we're trying to mix inheritance with value semantics, and C++ doesn't like that. See this SO question for some background. For example:

employee alice;
human atouk;
swap(atouk, alice);  // swap(human&, human&)

The above code compiles, since alice, being an employee, is-a human. However, we end up slicing away all the things that make alice an employee; Atouk takes over Alice's duties, and it's pretty much just a terrible state of affairs.

So, don't try to mix inheritance hierarchies with value semantics! They don't play well together.

However, if we trust our users to swap employees only with other employees, we can at least get the correct semantics via

friend void swap(employee& lhs, employee& rhs) {
    using std::swap;
    swap(static_cast<human&>(lhs), static_cast<human&>(rhs));
    swap(lhs.duty, rhs.duty);
    swap(lhs.title, rhs.title);
    swap(lhs.detail, rhs.detail);
}

So our whole employee class looks like this:

class employee : public human
{
  public:
    char *duty = nullptr;
    std::string title = "";
    details *detail = nullptr;

    friend void swap(employee& lhs, employee& rhs) {
        using std::swap;
        swap(static_cast<human&>(lhs), static_cast<human&>(rhs));
        swap(lhs.duty, rhs.duty);
        swap(lhs.title, rhs.title);
        swap(lhs.detail, rhs.detail);
    }

    employee() { /*...*/ }

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

    employee(employee&& f) {
        swap(*this, f);  // default-construct-and-then-swap
    }

    employee& operator=(const employee& f) {
        if (this != &f) {
            employee temp = f;  // copy-...
            swap(*this, temp);  // ...-and-swap
        }
        return *this;
    }

    employee& operator=(employee&& f) {
        swap(*this, f);  // move-by-swapping
        return *this;
    }

     ~employee() {
         delete [] duty;
         delete detail;
     }
 };