c++ - Does allocating memory to an empty class casues memory leak?

Question

The following function appears in the OctoMap code:

class AbstractOcTreeNode {}; -> They declare an empty class

AbstractOcTreeNode** children; -> This is declared in the OcTreeDataNode class header file

template <typename T>
void OcTreeDataNode<T>::allocChildren() {
  children = new AbstractOcTreeNode*[8];
  for (unsigned int i=0; i<8; i++) {
    children[i] = NULL;
  }
}

Wouldn't this cause memory leak? Shouldn't it be:

template <typename T>
void OcTreeDataNode<T>::allocChildren() {
  children = new AbstractOcTreeNode*[8];
  for (unsigned int i=0; i<8; i++) {
    delete children[i];
    children[i] = NULL;
  }
}

What am I missing? Thanks for the help!

Answer 1

You'd want to delete the entire array, not each of the individual array elements

template <typename T>
void OcTreeDataNode<T>::allocChildren() {
  children = new AbstractOcTreeNode*[8];
  for (unsigned int i=0; i<8; i++) {
    children[i] = NULL;
  }

  // .... later
  delete[] children ;
}

You must always match a new with a delete, and a new[] with a delete[], without mixing them.

For completeness (I'm guessing at the context) since the name of the function is allocChildren I assume it is their intention to new[] the array and not cleanup the memory, yet. Hopefully there would be a matching deallocChildren that would delete[] this memory later.

Answer 2

AbstractOcTreeNode** children;

children can be seen as an array of pointer values.

children = new AbstractOcTreeNode*[8];

We initialize it with an array of eight pointer values.

for (unsigned int i=0; i<8; i++) {
    children[i] = NULL;
}

Each of the eight children[i] pointer values is initially an uninitialized AbstractOcTreeNode*. We assign the NULL value to each of them. Calling delete beforehand on these uninitialized pointers would be Undefined Behavior.

There is only one memory allocation (new[] is only called once), and its result is kept in children. There is no leak as long as children is eventually cleaned up (using delete[], presumably in the destructor of OcTreeDataNode<T>).

Your confusion is the result of having multiple levels of pointers, at least some of which are owning. I personally also find this code hard to read because of that. In modern C++, you would not perform manual memory management, either for allocating the array of pointers (what about std::vector or std::array) or for the allocation of each AbstractOcTreeNode-dervied instance (which is not shown here). You might find std::vector<std::unique_ptr<AbstractOcTreenode>> children; in modern C++ instead.

Answer 3

What is the sense to allocate memory and at once to delete it?

template <typename T>
void OcTreeDataNode<T>::allocChildren() {
  children = new AbstractOcTreeNode*[8];
  for (unsigned int i=0; i<8; i++) {
    delete children[i];
    children[i] = NULL;
  }
} 

The function above does not make sense.

Pay attention to that an empty class has a non-zero size.

And there is allocated an array of pointers to an empty class. Objects of the class are not allocated in this function.

In this function

template <typename T>
void OcTreeDataNode<T>::allocChildren() {
  children = new AbstractOcTreeNode*[8];
  for (unsigned int i=0; i<8; i++) {
    children[i] = NULL;
  }
}

the variable children defined in a namespace gets the address of the allocated array in the member function.

So some other code is responsible to free the allocated memory.

In general it is a bad idea that a member function of a class uses a global variable.