0

I don't know how to properly have a vector of derived classes without memory leaks. I tried the following, but it has problems:

#include <iostream>
#include <vector>
using namespace std;

struct base {};

struct derived : public base
{
  derived() {}
};

struct Layer
{
  vector<base*> effects;

  Layer() {}

  ~Layer()
  {
    for(int ii = 0; ii < effects.size(); ii++)
    {
      cout << "called effect deleter" << endl;
      delete effects[ii];
    }
  }
};

int main()
{
  vector<Layer> layers;
  for(int i = 0; i < 10; i++)
  {
    layers.push_back(Layer());
    layers[i].effects.push_back(new derived());
    cout << i << endl;
  }
}

When I compile and run this code, I get the following output:

0
called effect deleter
1
called effect deleter
called effect deleter

I'm confused. Why does it only print 0 and 1 instead of 0 through 9? If I remove the Layer destructor, won't I have a memory leak? What is the proper way to handle this situation?

1
  • Layer is not following the Rule of 3/5/0, so it can cause problems when layers grows its capacity and has to make copies of existing Layer instances Commented Apr 23, 2020 at 3:15

2 Answers 2

1

If base *b, but *b is actually a derived, then delete b; is instant UB, because base::~base() is not virtual. Essentially, you try to delete the base part of *b (directed by the type of *b, which is base), but, because the destructor isn't virtual, you forget to destroy the derived part first. This leads to horrible things happening (probably some kind of stack corruption?). Fix it:

struct base {
    virtual ~base() = default;
};

Also, Layer::Layer(Layer const&) (the implicitly-defined copy constructor) is broken, since it duplicates the base* pointers from the argument. This copy constructor is called when std::vector<Layer> needs to resize its storage, which entails allocating a new hunk of contiguous memory and move-constructing new Layers out of the old ones, then destroying the old ones. Except a) Layer doesn't have a move constructor (the user-declared destructor prevents its generation), so "moving" Layers just copies them, and b) copying Layers is conceptually flawed, since when one Layer is destroyed it will delete all of its bases, and then the other Layer will try to delete them again later. Disable Layer copying and write its move.

struct Layer {
    std::vector<base*> effects;
    Layer() = default;
    Layer(Layer const&) = delete;
    Layer(Layer&&) = default;
    Layer &operator=(Layer const&) = delete;
    Layer &operator=(Layer &&other) {
        std::swap(this->effects, other.effects);
        // what used to be this->effects will be deleted when other is destroyed
        return *this;
    }

    ~Layer() {
        for(int ii = 0; ii < effects.size(); ii++) {
            std::cout << "called effect deleter\n"; // endl is usually unnecessary, and "\n" is portable
            delete effects[ii];
        }
    }
};

The moral is: use smart pointers :).

struct effect_deleter {
    void operator()(base *b) {
        std::cout << "called effect deleter\n";
        delete b;
    }
};
struct Layer {
    std::vector<std::unique_ptr<base, effect_deleter>> effects;
    // 3 constructors, 2 operator=s, and 1 destructor
    // all doing the right thing, "for free"
    // "Rule of 5" becomes "Rule of 0"
};

int main() {
    std::vector<Layer> layers;
    for(int i = 0; i < 10; i++) {
        layers.emplace_back(); // using push_back(Layer()) constructs a temporary Layer, then constructs the actual Layer in the vector by moving from the temporary; emplace_back just passes the arguments (here nothing) on to the constructor of Layer
        layers[i].effects.emplace_back(new derived()); // similar
        std::cout << i << "\n";
    }
}
Sign up to request clarification or add additional context in comments.

Comments

0

You cannot do this as a straight vector in c++ - different derived classes have different sizes, and as such, you get slicing when the copy constructors place elements in the array (classic object slicing).

The least painful way of doing this properly is by using a vector of shared_ptr's to the base class. It is possible to use an array of unique_ptr's too, but you have to be very careful with that, since things like range-based for-loops can move the ownership of your data into the temporary iteration variable.

3 Comments

Ok, but the OP knows that and is using vectors of pointers. Slicing is not what's happening. There are other things going wrong here.
Declare the destructor virtual
@jscmerge That's one thing. This question is a kaleidoscope of brokenness, though. That doesn't fix everything.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.