Memory leak from type structure created with new 2

[Mthales]

Hi there,
I have got a problem with a huge memory leak when I close down my program and I really can't track down how to stop it. I know it's probably me missing something obvious but please any advice or help will be greatfully recieved.

I have a simple ordered tree structure that is made up of nodes of the following type:

typedef struct attrib_node
{
	/* pointers to subtrees */
	struct	attrib_node  *left, *right;
	CString Name;
	CString Value[MAX_STATES];
} Attrib_node;

I create nodes very simply like this when I'm building the tree structure from my data:

Attrib_node *new_nodep;
new_nodep = new Attrib_node;

When I have finished with them I have tried to free or delete the nodes but I can't seem to get them to clear away nicely. I either get memory access exceptions or the same old memory leak.

I know that if I had these nodes as classes I could change the constructor and destructor functions but what do I do since they are only structures?

Thanks in advance
M

 

[PerFnurt]

>When I have finished with them I have tried to free or delete

Which one is it? free or delete?

You should always use delete when using new.

>I either get memory access exceptions

Then maybe you delete the same object twice?

>or the same old memory leak.

Then you don't delete it at all.

>I know that if I had these nodes as classes I could change the constructor and destructor functions but what do I do since they are only structures?

You can do that for structs as well.
The only difference between a class and a struct is the access scope. It is by default public for structs and private for classes.

/Per
[sub]
www.perfnurt.se[/sub]

 

[ArkM]

You forget: struct and class in C++ are (nearly) synonyms (a struct is a class with all public members). You may add constructors and destructor to the struct.
You can't free new_nodep (with free()) obtained by new op: you must delete it (to call a default Attrib_node destructor).
May be you have troubles with delete order (when delete a whole tree)?..

 

[Mthales]

Thanks for those points guys.

PerFnurt I tried both free and delete cause I counldn't seem to make it work. But I see what you mean I created new so I must use delete. I've put it back to just do this and I get an access violation but this could be as you sugest I'm trying to delete the same item twice.

ArkM How may I add constructors and destructors to the struct? Also that delete order could well be the problem causing me to try deleting nodes over again.

I have a function that recursivly goes down the tree to delete the "leaves" and ripples back up destroying it all. Could you please take a look and point me to where it's going wrong? I just pass in the "top" of the tree.

void DestroyAttribs(Attrib_node* np){
	if(np == NULL) return;
	if(np->left != NULL) DestroyAttribs(np->left);
	if(np->right != NULL) DestroyAttribs(np->left);

	delete np;
	np = NULL;	
}

Thanks for your help
M

 

[PerFnurt]

>How may I add constructors and destructors to the struct

Now, we both have said that struct and class is virtually the same thing. How would you add con- & destructors to a class?

struct Foo
{
  Foo() { cout << "Foo's default constructor"; }
  ~Foo() { cout << "Foo's destructor"; }
};

>could be as you sugest I'm trying to delete the same item twice.

Yeahm I think so. Specially since

if(np->right != NULL) DestroyAttribs(np->left);

should be

if(np->right != NULL) DestroyAttribs(np->right);

/Per
[sub]
www.perfnurt.se[/sub]

 

[PerFnurt]

By the way, I see you do assign np NULL after delete, that is good, but what about the thingie that point to it?

Ie

if(np->left != NULL) DestroyAttribs(np->left);
// At this point np->left is still != NULL
// The np = NULL assignment is done to the local np parameter only.
// You could fix that by making the param a reference
void DestroyAttribs(Attrib_node*& np)

/Per
[sub]
www.perfnurt.se[/sub]

 

[IonelBurtan]

1. The above signatures function should be:

void DestroyAttribs(Attrib_node** np) or
void DestroyAttribs(Attrib_node*& np)

You are passing the np pointer by-value. Then when the function exits, the np pointer WILL NOT be NULL even though you make np=NULL. (rememeber the passing by-value rules).

Therefore the code will try to delete the same node twice causing the exception.


2. How to look for memory leaks:

Read the following topic from MSDN"
Detecting and Isolating Memory Leaks Using Microsoft Visual C++

This will give you a several method to track memory leaks using the CRT functionality. the simplest is to use _CrtSetDbgFlag. If you use #define _CRTDBG_MAP_ALLOC you will also see the line where you made the allocation.

good luck,



s-)

Blessed is he who in the name of justice and goodwill, sheperds the weak through the valley of darkness...

http://www20.brinkster.com/ionelb

 

[cpjust]

If you're gonna add constructors & destructors, you might as well make it a class. I just think structs with functions look strange & a little confusing...

Why not do something like this:

class Attrib_node
{
public:
    Attrib_node()
    :  left( NULL ), right( NULL )
    {}

    ~Attrib_node()
    {
        // Recursively delete all attached nodes.
        if ( left != NULL )
            delete left;

        if ( right != NULL )
            delete right;
    }

    /* pointers to subtrees */
    Attrib_node  *left, *right;
    CString Name;
    CString Value[MAX_STATES];
};

 

[Mthales]

Thank you all so much for your help.

I can't believe I made that mistake of passing by value (and the very dumb typo of checking right and going left :~/) I've changed these now and it's fixed the memory exception.

However since the memeory leak is still there when I close down the program I'm putting in the methods sugested by IonelBurtan but this says:

Detected memory leaks!
Dumping objects ->
strcore.cpp(118) : {3373} normal block at 0x014C3C80, 18 bytes long.
Data: < spar> 01 00 00 00 05 00 00 00 05 00 00 00 73 70 61 72
strcore.cpp(118) : {3372} normal block at 0x014C3C28, 18 bytes long.
Data: < spar> 01 00 00 00 05 00 00 00 05 00 00 00 73 70 61 72
etc.

So am I right in guessing this means that my use of CStrings is going wrong?
Will the delete of my node structure destroy the array of CStrings correctly or do I have to put in a destructor function?

M

 

[IonelBurtan]

Normally, the CString array should distroy itself if you do not add members with new or malloc. (e.g. it is on the stack)

But in order to avoid all this mess, I suggest you use the CStringArray class instead of an array of CStrings. You will find it more conveninent and you do not have to know the size of the CString array or make assumption about it. also the size of your object will significally decrease.

One more tip: double clicking on an the text of a memory leak should lead you in the code to the exact line that made the allocation

HTH,

s-)

Blessed is he who in the name of justice and goodwill, sheperds the weak through the valley of darkness...

http://www20.brinkster.com/ionelb

 

[PerFnurt]

I think using destructors etc as suggested by cpjust will make your code easier to follow and thus easier to debug, specially in situations like this.

But part from that, your "outside deletion" should work anyway.

Obviously some instance isn't deleted...hmmm...are you for example sure you delete the root?

And to add another aspect i would recommend std::vector<CString> rather than CStringArray (but then maybe its just my allergy to MFC's collecitons).

This is not really the problem though - when Attrib_node is deleted the CStrings should clean up as well.

If they don't - then you're simply missing the delete of Attrib_node instance(s).


/Per
[sub]
www.perfnurt.se[/sub]

 

[Mthales]

OK I've implemented using the CStringArray and I went back and double/tripple checked the destroy function and it seems to be working now. Looks like I was messing up my pointers slightly where I'd changed it to a by reference parameter.

Thank you all for all your help, I've made progress today and that's all thanks to you guys.

M