Deleting Pointer shared with other class

Question

I'm having problems with memory deallocation in c++ using pointer shared between classes.

An example:

My vertices are defined as:

class Vertex{
    double x;
    double y;
}

A square is defined as:

class Square{
    Square(Vertex* a, Vertex* b, Vertex* c, Vertex* d);
    ~Square(); // destructor
    Vertex* a;
    Vertex* b;
    Vertex* c;
    Vertex* d;
}

My destructor is implemented so:

Square::~Square(){
    delete a;
    delete b;
    delete c; 
    delete d;
}

My squares are stored into std::vector<Square*> squares, so for clean all my memory I do:

for(unsigned int i = 0; i < squares.size(); i++){
    delete(squares.at(i));
}

So what is the problem? If two squares share a Vertex, my program crash because it is trying to delete a pointer that no longer exists. How can I solve this problem?

Answer 1

It seems to me that you are coding in C++ with a Java-like mindset. Vertex objects that contain just two doubles (e.g. the X and Y components as in your case) are better stored on the stack, without pointer indirection. So, I would declare the Square class like this:

class Square{
...
  Vertex a;
  Vertex b;
  Vertex c;
  Vertex d;
};

If, instead of embedding Vertex objects you want a kind of reference mechanism, you could store the vertexes in a std::vector<Vertex> array, and store in the Square class integer indexes to vertex positions in the array.

If you really want shared ownership semantics with pointers, then consider using a smart pointer like std::shared_ptr. No explicit delete: shared_ptr will automatically release memory when the reference count reaches zero.

In this case, replace the raw Vertex* owning pointer data members with shared_ptr<Vertex> inside your Square class. Moreover, remove the destructor code from your Square class, as shared_ptr knows how to delete itself.

In your Square class constructor you can take the shared_ptr<Vertex> smart pointers by value, and std::move them inside the corresponding data members, e.g.:

Square::Square(
  std::shared_ptr<Vertex> pa,
  std::shared_ptr<Vertex> pb,
  std::shared_ptr<Vertex> pc
)
  : a{std::move(pa)}
  , b{std::move(pb)}
  , c{std::move(pc)}
{}

Replace also vector<Square*> with vector<shared_ptr<Square>> (but, again, are you sure that a simpler vector<Square> doesn't serve you well?), and use std::make_shared to create the smart pointers.

Answer 2

You should never allocate memory manually in Modern C++, unless you have a very good reason to do so. If your Square class wants to model shared ownership of Vertex instances, then you should use std::shared_ptr:

class Square{
    // ...
    std::shared_ptr<Vertex> a, b, c, d;
}

I however advise you to reconsider using dynamic memory allocation for something as simple and lightweight as a Vertex.