0

I have 4 classes: 1 Base, 2 Deriveds and 1 Container class. The Container class holds a vector of Base pointers.

I want to create a copy constructor for my class Container that doesn't cast the Derived pointers to a Base, so that I can cast the Base pointers to Derived pointers afterwards.

class Base {
   int m_base_attribute;
public:
   Base() : m_base_attribute(420) {}

   virtual void say_hello() {
      std::cout << "Hello !" << std::endl;
   }
};

class Derived : public Base {
   int m_derived_attribute;
public:
   Derived() : Base(), m_derived_attribute(69) {}

   virtual void say_hello() {
      std::cout << "I'm a derived class !" << std::endl;
   }
};

class Container {
   std::vector<Base*> m_base_vector;
public:
   Container() {
      m_base_vector.push_back(new Derived());
   }

   Container(const Container& model) {
      for(auto base : model.m_base_vector){
         m_base_vector.push_back(new Base(*base));
      }
   }

   ~Container() {
      for(auto base : m_base_vector) {
         delete base;
      }
   }
};

Is there a way to do it without any memory leaks?

L. F.
  • 19,445
  • 8
  • 48
  • 82
  • 2
    Your problem isn't memory leaks, it's slicing. (Also, lack of virtual destructors, but that may just be your example code.) You need a virtual `clone()` function in your base class that correctly copies the object. – Sebastian Redl Dec 08 '19 at 11:18
  • Ah okay. And will it use the right version of the function intelligently, or do I have to cast it to something ? – Rick Anaconda Dec 08 '19 at 11:28

1 Answers1

2

The problem is that new Base(*base) always creates a Base object, never a Derived object. This is called slicing. The workaround is to use a virtual clone function and a virtual destructor:

class Base {
    int m_base_attribute;
public:
    // ...
    virtual std::unique_ptr<Base> clone() const
    {
        return std::make_unique<Base>(*this);
    }
    virtual ~Base() {}
};

class Derived : public Base {
    int m_derived_attribute;
public:
    // ...
    std::unique_ptr<Base> clone() const override
    {
        return std::make_unique<Derived>(*this);
    }
};

Note that I used std::unique_ptr instead of raw pointers to avoid memory leaks. Now you can implement the Container class without slicing:

class Container {
    std::vector<std::unique_ptr<Base>> m_base_vector;
public:
    // ...    
    Container(const Container& model)
    {
        m_base_vector.reserve(model.m_base_vector.size());
        for (const auto& p : m_base_vector) {
            m_base_vector.push_back(p->clone());
        }
    }
};
L. F.
  • 19,445
  • 8
  • 48
  • 82
  • The way you have declared `clone()` means that objects can still be sliced - all it takes is for one derived class to not override the `clone()` function. That type of error can be avoided by making `Base` an abstract class, with the `clone()` function pure virtual. That forces derived classes to override the `clone()` (otherwise the derived class cannot be instantiated) and helps prevent unintended slicing. – Peter Dec 08 '19 at 12:45
  • @Peter That doesn’t solve the problem completely though - I can still derive from a derived class and forget to implement clone(). – L. F. Dec 08 '19 at 12:49
  • While that's true, it is also a reason that a lot of design and coding guidelines specifically disallow deriving from non-abstract classes. – Peter Dec 08 '19 at 12:51
  • @Peter While that’s also true, there is also a lot of code that does not follow such guidelines. So I’ll leave this decision to the OP. – L. F. Dec 08 '19 at 12:54