Deleting old dynamically allocated memory in assignment operator overloading method in c++

Question

I have two questions which are connected to each other.

I am implemeting a region quad-tree structure. In this QTree class, I have some fields and more importantly a

vector<QTree*> quads; 

field that holds four divided regions of current square. Firstly, I want to implement a destructor for this class. Note that I cannot define anything in public field in the class (If I were, this was a piece of cake!) so I did this:

QTree::~QTree ()
{
  if ( quads [ 0 ] != NULL )
  {
     for ( int i = 0; i < 4; i++ )
     {
        delete ( quads [ i ] );
     }
  } 
}

According to valgrind, this works ie. no memory leak and error but I am not very sure if it is. Could you state your opinion about destructor's validity?

Second issue is that, in the implementation of overloaded assignment operator of this class if I write after self-assignment check and before anything else in this method this:

delete this;

I take

* Error in ` ': double free or corruption (out): 0x00007fffcb4d46d0 * : line 3: 4227 Aborted (core dumped) leakcheck

error.

If I write (same code in destructor.)

if ( quads [ 0 ] != NULL )
    {
        for ( int i = 0; i < 4; i++ )
        {
            delete ( quads [ i ] );
        }
    }

instead "delete this;" in overloaded assigment operator, I get no error. What is wrong, could you clarify me?

EDIT: I removed copy constructor.

Maybe the problem is using self destruction of an object. Here are some useful information on this: https://isocpp.org/wiki/faq/freestore-mgmt#delete-this

Answer 1

In modern C++, unless you are in very specific cases, you should manage owning pointers using smart pointer classes, like std::unique_ptr or std::shared_ptr.

Note that STL smart pointers can be nicely combined with STL containers, making it possible to implement a kind of "deterministic (reference-count-based) garbage collector".

In your case, I'd consider using:

// Supports copy semantics
std::vector<std::shared_ptr<QTree>> quads;

or:

// This is movable but not copyable
std::vector<std::unique_ptr<QTree>> quads;

depending on your needs and on your design.

In this way, you can enjoy the so called "Rule of Zero": the C++ compiler will automatically implement copy constructor, move constructor, copy assignment, move assignment and destructor for you.

Answer 2

1. delete in C++ specifically allows 0/nullptr as an argument, so your test is superficial
2. Safer to write deallocation loop up to actual vector size for ( int i = 0; i != quads.size(); i++ )

3. What is wrong, could you clarify me?. When you call delete this you not only call to destructor (with quads deallocation), you then dellocate memory of your class, literally taking it out. If class is allocated on the stack, you'll get even more problems. Solution - refactor deallocation and call it from anywhere

   private void QTree::deallocate() {
     for(int i = 0; i != quads.size(); ++i) {
        delete (quads[i]);
     }
   }
   
   QTree::~Qtree() {
       deallocate();
       ...
   }
   
   QTree& QTree::operator=(const QTree& rhs) {
       deallocate();
       ...
   }
   

Answer 3

if I understand correctly, what you are trying to do is:

QTree& operator =(const QTree& rhs) {
    // destroy current content
    delete this;
    // clone and copy content of rhs to current quads
    if (rhs.quads[0] != NULL) {
        for (int i = 0; i < 4; ++i) {
            quads[i] = new QTree(*rhs.quads[i]);
        }
    }
    return *this;
}

So that you can avoid duplicating the code in the destructor

First thing: why delete this is not working as you expect: that is because after calling the object destructor (which is want you want to do), delete this will also free the memory of the object. Your own current object, the one you are going to use to assign copied values from the other object. Calling the destructor without freeing the memory is possible using this->~Qtree()

In fact, it is also possible to call a constructor without allocating memory using the placement new operator, so if you already have a copy constructor, you assignment operator could be:

QTree& operator=(const QTree& rhs) {
    this->~QTree();
    new(this)(rhs);
    return *this;
}

Looks neat, but unfortunately, it is not recommended to do this as it is not exception safe: if the constructor throws an exception, your object is going to be in a bad state.

One of the recommended way to implement the assignment operator is to use a nothrow "swap" method or function:

QTree& operator=(const QTree& rhs) {
    QTree tmp(rhs);
    swap(tmp)
    return *this;
}

Now that I answered the question, there are a few other issues with your code, especially it seems quite unnecessary to use a variable length vector to always store 4 items and always 4 items. Why no simply a QTree[4] array?