Class cross deletion

Rok-Kralj
Rok-Kralj used Ask the Experts™
on
Consider the code below. I get the folowing error and I know why i get it. The question is, how can i avoid it with the similar functionality and without memory leaks?

g++ -Wall -o "test" "test.cpp" -O3 -funroll-loops
test.cpp: In destructor ‘a::~a()’:
test.cpp:10: warning: possible problem detected in invocation of delete operator:
test.cpp:10: warning: invalid use of incomplete type ‘struct b’
test.cpp:1: warning: forward declaration of ‘struct b’
test.cpp:10: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.
Compilation finished successfully.

struct b;

struct a {
	b* ptr;
	
	a() {
		this->ptr=0;
	}
	~a() {
		delete this->ptr;
	}
};

struct b {
	a* ptr;
	
	b() {
		this->ptr=0;
	}
	~b() {
		delete this->ptr;
	}
};

Open in new window

Comment
Watch Question

Do more with

Expert Office
EXPERT OFFICE® is a registered trademark of EXPERTS EXCHANGE®
Top Expert 2009

Commented:
Place the allocation (new) and de-allocation (delete) in the same place.

Right now, you allocate memory in a different location than where you de-allocate it, and that causes your problem.
Top Expert 2009

Commented:
Could you show how you allocate memory for the structs ? And where you instantiate and destroy them ?
Hi Rok-Kralj,

usually such circular dependencies can be eliminated by a better design approach.
If you want, we can try to find a better design and eliminate the circular dependency.

In order to do that, we would need more information about the original classes. Could you post them?

ike
Exploring SQL Server 2016: Fundamentals

Learn the fundamentals of Microsoft SQL Server, a relational database management system that stores and retrieves data when requested by other software applications.

Author

Commented:
No, that is a concept, a minimal case of something in a big application.

About that one who suggested placing news and deletes together, I can't do that... I new in let's say 3 places, then use that pointers for some time and then i want to delete them (because the structures are nested) and by deleting one i want the destructor to be called on all its "childs".

Let's say:
struct b;

struct a {
	vector<b*> params;
	
	a() {
	}
	~a() {
		int size=this->params.size();
		for (int i=0; i<size; ++i) {
			delete this->params[i];
		}
	}
};

struct b {
	vector<a*> params;
	
	a() {
	}
	~a() {
		int size=this->params.size();
		for (int i=0; i<size; ++i) {
			delete this->params[i];
		}
	}
};

Open in new window

Author

Commented:
I cannot change the design, because let's say i have class "brace" and "expression". Brace can contain expression and expression can contain multiple braces.
Top Expert 2009

Commented:
>> I new in let's say 3 places

The point is that one entity should be responsible for an allocated block of memory. If you transfer this responsibility like you do, then you find yourself in this kind of situation.

Choose the responsible for each block of memory, and stick to it. For example, let the a struct be responsible for (de-)allocating the b structs in its vector. And let someone else be responsible for (de-)allocating the a structs (in other words, don't let the b struct de-allocate any of the a structs - it's not its responsibility).


To give you more specific advice, we'll need to see a more complete representative sample.
Top Expert 2009

Commented:
>> Brace can contain expression and expression can contain multiple braces.

Ok. From that description, I gather that there is a tree-like hierarchy of structs, where each can have children, etc.

A tree is perfectly suited for memory allocation responsibilities. Let each parent node be responsible for (de-)allocating its children.
You mentioned that you got errors, but in fact they are only warnings. I got identical warnings in Cygwin. However, with a little effort, I was able to get errors. Perhaps the 5 files below can help - they have no warnings or errors in Cygwin.

$ g++ a.cpp b.cpp destructor.cpp
OUTPUT:
$ ./a
 a()
 b()
~b()
~a()


In UML you see things like:

             *                         1
classA <-------------------> classB

So, both classes have an association with the other class. A one to many association. If using a UML code generator, you may get different formulations of 1 vs. many, but your approach seems familiar enough.
///////////////////
// FILENAME: a.h
///////////////////

#ifndef A_H
#define A_H

struct a;

#include "b.h"

struct a {
	b* ptr;
	
	a();
	~a();
};

#endif


///////////////////
// FILENAME: b.h
///////////////////

#ifndef B_H
#define B_H

struct b;

#include "a.h"

struct b {
	a* ptr;
	
	b();
	~b();
};
#endif


///////////////////
// FILENAME: a.cpp
///////////////////

#include <iostream>
using namespace std;

#include "b.h"

a::a() {
   cout << " a()" << endl;
   this->ptr=0;
}
a::~a() {
   cout << "~a()" << endl;
   delete this->ptr;
}


///////////////////
// FILENAME: b.cpp
///////////////////

#include <iostream>
using namespace std;

#include "a.h"

b::b() {
   cout << " b()" << endl;
   this->ptr=0;
}
b::~b() {
   cout << "~b()" << endl;
   delete this->ptr;
}


///////////////////
// FILENAME: destructor.cpp
///////////////////

#include "a.h"
#include "b.h"

int main() {
   {
      a myA;
      b myB;
   } // force destructor call
   return 0;
}

Open in new window

Author

Commented:
Sample:

Class brace:
vector<param*>;


Class param:
brace* child=0;
number=0;


And then i initalize the structure (1,(2, 3, (1, 2)),3)

use it some time....

and then I want to free the memory.

Author

Commented:
You made a good point!
I was working on the modifications w.r.t. vector, but I see you are now satisfied.

I just wanted to correct some UML terminology. Since the lifetime of your "has a" object is controlled by the container, then you have a Composite relationship. I suspect you would get into trouble when destroying if you literally had one object containing another which then contained the container. But since you are tree-based, I think you will be OK.
Top Expert 2009

Commented:
With that approach, you still have to be extremely careful about who's responsible for which block of allocated memory. It is very easy to mess up if you don't properly define and enforce memory responsibilities.

I would recommend that you at least consider a less error-prone approach. Like having a base class that takes care of managing the children, and derived classes for the specific Brace, Param, etc. functionality. Make sure to explicitly transfer control to the parent when adding a child (by making a deep copy eg.).
Isn't the intention to free up memory simply by deleting the root node of the tree?
Top Expert 2009

Commented:
Only if you can be sure that the root node is (directly or indirectly) responsible for all memory in the tree. And that is not the case right now. Right now, anyone could be responsible, and more than one entity can potentially try to delete the same object (because it's not clear where the responsibility lies).
Good point about safety. I wasn't sure exactly why you indicates that this "is not the case right now". I didn't notice in the discussion on how the author's classes were going to be integrated into the project. It could be that the Tree class simply contains a Brace class. But the author is welcome to pursue this point if desired.
Top Expert 2009

Commented:
Anything is possible.

But my point is, that with a proper memory responsibilities design, this circular dependency problem would never have arisen. I've tried to explain that from the beginning, but apparently the point didn't make it across.

In other words, the real issue hasn't been solved - the symptoms have been treated instead.

Author

Commented:
Yes, all the classes are in a tree structure, meaning no cycles.

There are multiple "root nodes", but each one is managed and later deleted separately.

So, it is true that if call the destructor of a root node, then the destructor of all it's children will be called and so on?

Author

Commented:
It is that

brace contains multiple params in a vector, it's destructor then deletes every element of the vector in a for loop.

Param contains pointer to another brace or simply is a "leaf node" = has a number and null pointer.
Top Expert 2009

Commented:
As long as you're very careful. I'd still prefer to enforce this in the code. Mistakes are easily made. Furthermore, a design that depends on circular dependencies is often fragile and difficult to maintain.
@Rok-Kralj,
Feel free to re-open this question if you believe it will help you. I have to go now. But my own experience with cascading deletions in trees (no cycles) seemed to be straightforward. I may not be fully understanding exactly what others are proposing, so reopening this question for fine details may prove beneficial.

Especially since I was using Rhapsody for C++ with auto-code generation, I saw frequent bidirectional associations with constructions similar to what I posted. This was a large 20 developer project all using Rhapsody on one model. I don't recall any issues arising from the circular dependencies. However, it can be tricky especially for the novice, which I see that you are not.

If you post actual classes, we can help verify that your memory management is sound, and others may make concrete suggestions to remove circular dependencies. When I joined the large Rhapsody project, much of the design and framework was in place. Removing circular dependencies, practically speaking, would have been an expensive proposition if it turned out to be difficult to work with. BTW, on this project, every detail of the design, and every line of code was scrutinized and approved not only internally, but also by outside agencies.
Top Expert 2009

Commented:
I don't see a reason to re-open the question. I'm more than happy to continue discussing this if needed.

phoffric, I think the misunderstanding may lie in the difference between memory management responsibilities (which include both allocation and de-allocation of objects under an entity's control), and cleanup responsibilities (which only deal with de-allocation, not allocation). I'm talking about the former.

It's hard enough to get memory management right in a small application - it becomes extremely hard in a bigger applications with many interacting components, and different teams working together. It is vital to have a proper memory management design in order to keep it maintainable, and to avoid many issues.

If you make it very clear that the responsibility passes to the container, and everyone acts accordingly, then of course you can make it work. But it would be a lot easier if the code enforces this. That way there's no way that people can "forget".

One last note about circular dependencies like the one in the question : this is easily resolved by using one class to manage the tree structure, instead of spreading out that responsibility over a multitude of different node types. See my earlier suggestion.

Author

Commented:
Neither do I see a reason to reopen the question. I'll stick by my decision.

Thanks to all participants, but you, phoffric, helped most by reminding me that a destructor is afterall just a function and can be put outside the class and that way i resolved the dependency problem.

I'll stick to my design. Thanks again to both.

Author

Commented:
And I forgot - You two have shown a great example of a good, constructive debate. Thanks!

Do more with

Expert Office
Submit tech questions to Ask the Experts™ at any time to receive solutions, advice, and new ideas from leading industry professionals.

Start 7-Day Free Trial