Object returned from lambda loses property value

Question

I am taking a course that is several years old so learning how to use serialization for the first time.

When the "return result" executes in the lambda, the Address property of the Contact becomes uninitialized. The commented out code works just fine so I am fairly sure that I compiled the Boost libraries ok.

The name on the Contact comes back ok. Why doesn't the Address?

#include <string>
#include <iostream>
#include <memory>
#include <functional>
#include <sstream>
using namespace std;

#include <boost/serialization/serialization.hpp>
#include <boost/archive/text_iarchive.hpp>
#include <boost/archive/text_oarchive.hpp>

struct Address
{
public:
    string street, city;
    int suite;

    Address() {};
    Address(string street, string city, int suite)
        : suite(suite),street(street),city(city){ }

  friend ostream& operator<<(ostream& os, const Address& obj)
  {
    return os
      << "street: " << obj.street
      << " city: " << obj.city
      << " suite: " << obj.suite;
  }

private:
  friend class boost::serialization::access;

  template<class Ar> void serialize(Ar& ar, const unsigned int version)
  {
    ar & street;
    ar & city; 
    ar & suite;
  }
};

struct Contact
{
  string name;
  Address* address;


  friend ostream& operator<<(ostream& os, const Contact& obj)
  {
    return os
      << "name: " << obj.name
      << " address: " << *obj.address;
  }
private:
  friend class boost::serialization::access;

  template<class Ar> void serialize(Ar& ar, const unsigned int version)
  {
    ar & name;
    ar & address;
  }
};

int main()
{
  Contact john;
  john.name = "John Doe";
  john.address = new Address{ "123 East Dr", "London", 123 };

  auto clone = [](Contact c)
  {
    ostringstream oss;
    boost::archive::text_oarchive oa(oss);
    oa << c;

    string s = oss.str();

    Contact result;
    istringstream iss(s);
    boost::archive::text_iarchive ia(iss);
    ia >> result;
    return result;
  };

  // This works fine
  //ostringstream oss;
  //boost::archive::text_oarchive oa(oss);
  //oa << john;

  //string s = oss.str();

  //Contact newJane;
  //{
     // istringstream iss(s);
     // boost::archive::text_iarchive ia(iss);
     // ia >> newJane;
  //}

  //newJane.name = "Jane";
  //newJane.address->street = "123B West Dr";

  //cout << john << endl << newJane << endl;


  Contact jane = clone(john);
  jane.name = "Jane";
  jane.address->street = "123B West Dr";

  cout << john << endl << jane << endl;

  getchar();
  return 0;
}

Answer 1

Contact does not overload the copy constructor. Hence, the default is generated.

The default copy constructor copies all (non-static) member variables using their default constructors. Specifically, the default constructor of a pointer just copies the address which is stored.

So, using the copy construction of Contact, there is constructed a new instance with Contact::address pointing to the exact same Address like the original instance.

Hence, changing the address of jane will change the address of joe as well.

This may be intended or not:

• intentional to share resources
• unintentional if Contact shall have exclusive ownership of its address.

If jane and joe are not married this might be unintended.

In its current state, the design has another flaw:

Which instance is responsible to delete the address pointee when a Contact is destroyed?

If it would be added to the destructor ~Contact() things start to become worse. (Deleteing jane would delete her address and leaving john with a dangling pointer.)

As it is now, destroying a Contact may produce a memory leak. (Outside code had to be responsible to delete left instances of Address. This is hard to maintain.)

Such design issues are not rare and leaded to the Rule of three which says:

If one of

• destructor
• copy constructor
• copy assignment operator

is explicitly defined then the other most probably too.

With C++11 (introducing move semantics), this was extended to the Rule of Five adding

• move constructor
• move assignment operator.

Thereby, one explicit definition may be simply to delete them:

struct Contact {
  Address *address;

  // default constructor with initialization to empty
  Contact(): address(new Address()) { }

  // constructor with values
  Contact(const Address &address): address(new Address(address)) { }

  // destructor.
  ~Contact()
  {
    delete address; // prevent memory leak
  }

  // move constructor.
  Contact(Contact &&contact): address(contact.address)
  {
    contact.address = nullptr; // prevent two owners
  }
  // move assignment.
  Contact& operator=(Contact &&contact)
  {
    address = contact.address;
    contact.address = nullptr; // prevent two owners
    return *this;
  }

  // prohibited:
  Contact(const Contact&) = delete;
  Contact& operator=(const Contact&) = delete;
};

This is an improvement concerning memory management but counter-productive concerning the intention to clone() instances of Contact.

Another possible solution is to store address as std::shared_ptr<Address> instead of Address*. std::shared_ptr (one of the smart pointers) has been introduced for such kinds of problems (concerning shared ownership).

struct Contact {
  std::shared_ptr<Address> address;

  // default constructor with initialization to empty
  Contact(): address(std::make_shared<Address>()) { }

  // constructor with values
  Contact(const Address &address):
    address(std::make_shared<Address>(address))
  { }

  // another constructor with values
  Contact(const std::shared_ptr<Address> &address):
    address(address)
  { }

  // destructor.
  ~Contact() = default;
  /* default destructor of shared_ptr is fully sufficient
   * It deletes the pointee just if there are no other shared_ptr's to it.
   */

  // copy constructor.
  Contact(const Contact&) = default; // copies shared_ptr by default
  // copy assignment.
  Contact& operator=(const Contact&) = default; // copies shared_ptr by default
  // move constructor.
  Contact(Contact&&) = default;
  // move assignment.
  Contact& operator=(Contact&&) = default;
};

Setting the "Five" to default is in this case effectively the same like leaving them out.

---

Links I found while checking not to write anything stupid:

• SO: Defaulted constructor vs implicit constructor
• SO: Is a =default move constructor equivalent to a member-wise move constructor?

Answer 2

There's no compelling reason to use pointers to Address in Contact, so don't. This also means that the compiler generated copy constructor can replace clone.

struct Contact
{
  string name;
  Address address;

  friend ostream& operator<<(ostream& os, const Contact& obj)
  {
    return os
      << "name: " << obj.name
      << " address: " << obj.address;
  }
private:
  friend class boost::serialization::access;

  template<class Ar> void serialize(Ar& ar, const unsigned int version)
  {
    ar & name;
    ar & address;
  }
};

int main()
{
  Contact john;
  john.name = "John Doe";
  john.address = Address{ "123 East Dr", "London", 123 };

  Contact jane = john;
  jane.name = "Jane";
  jane.address.street = "123B West Dr";

  cout << john << endl << jane << endl;

  getchar();
  return 0;
}