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!

`new AbstractOcTreeNode*[8]` just creates the array of pointers. The pointers contained in that array don't point to any instances of `AbstractOcTreeNode` yet. — François Andrieux, Dec 12 '19 at 16:41
In reality there are 100 lines of code between those curly braces, among them are virtual member functions. It is not an empty class. — n. m. could be an AI, Dec 12 '19 at 17:41

Cory Kramer · Answer 1 · 2019-12-12T16:43:38.953

0

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.

edited Dec 12 '19 at 16:43

answered Dec 12 '19 at 16:41

Cory Kramer

114,268
16
167
218

Really? `children` appears the be a member variable and a function called `allocChildren` that frees the memory on exit seems very misleading. – NathanOliver Dec 12 '19 at 16:42
@NathanOliver-ReinstateMonica That's what I thought too. Added a comment to address this. – Cory Kramer Dec 12 '19 at 16:44
I'd expect the cleanup to happen in `OcTreeDataNode::~OcTreeDataNode()` TBH. – MSalters Dec 12 '19 at 16:46
actually it metnions: The tree class manages this pointer, the array, and the memory for it! – RonD Dec 12 '19 at 17:01

Max Langhof · Answer 2 · 2019-12-12T16:49:56.793

0

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.

edited Dec 12 '19 at 16:49

answered Dec 12 '19 at 16:43

Max Langhof

23,383
5
39
72

"Each children[i] pointer value is initially an uninitialized AbstractOcTreeNode*". So why do they set children[i] = NULL; for each child. Furthermore, so if I want to actually set a new child later on to be a OcTreeDataNode obkect, I should use: children[i] = new OcTreeDataNode; ? – RonD Dec 12 '19 at 16:57
Also, would this be the right way to go if AbstractOcTreeNode wasn't an empty class? i.e children = new AbstractOcTreeNode*[8]; just allocates pointers without allocating space for the class objects? so I can't use children[i], before I set children[i] = new AbstractOcTreeNode; ? – RonD Dec 12 '19 at 17:04
@RonD Setting it to `NULL` is preferable to leaving it uninitialized. Dereferencing a `nullptr` is very likely to crash the program immediately, dereferencing a random memory value leads to chaos and pain. Plus, calling `delete` on a `nullptr` is fine and does nothing, whereas calling `delete` on an uninitialized value is UB. And yes, `new OcTreeDataNode` could be one way to obtain a pointer to put in there. What does the documentation say? – Max Langhof Dec 12 '19 at 17:05
Your second comment is correct. `new AbstractOcTreeNode*[8]` just allocates space to hold eight pointer values. – Max Langhof Dec 12 '19 at 17:05

score 0 · Answer 3 · answered Dec 12 '19 at 16:47

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.

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

3 Answers3