is this a memory leak?

G

gianpaolof

Hello!

I was looking at the following piece of code:

void MyClass::myMethod( )
{
MyItem *item = NULL;


item = new MyItem( ..., iconView, ..., ...);
}

I thought it was necessary to delete the item before the end of
myMethod scope to avoid
memory leak.

Then I looked at the MyItem constructor:

class MyItem : public MyIconViewItem
{
public:
MyItem(..., MyIconView *parent, ..., ...);
}

Documentation for MyIconViewItem says:
When the MyIconView is deleted, all items in it are deleted
automatically.

Can I assume that the memory above is deallocated when MyIconView is
deleted?


Best regards.
Gianpaolo
 
J

Jakob Bieling

void MyClass::myMethod( )
{
MyItem *item = NULL;


item = new MyItem( ..., iconView, ..., ...);
}

I thought it was necessary to delete the item before the end of
myMethod scope to avoid
memory leak.

Then I looked at the MyItem constructor:

class MyItem : public MyIconViewItem
{
public:
MyItem(..., MyIconView *parent, ..., ...);
}

Documentation for MyIconViewItem says:
When the MyIconView is deleted, all items in it are deleted
automatically.

Can I assume that the memory above is deallocated when MyIconView is
deleted?

This depends on whether the item you created above is automatically
an item of 'iconView'. If it is, and the documentation does not lie, you
should be able to rely on it being deleted.

But what if you create an automatic variable of type 'MyItem'? Like
"MyItem item (..., iconView, ..., ...);"? If 'iconView' will 'delete
automatically' as it said, you will run into big trouble with this one.

I guess the above is a perfect example showing why code that
allocates objects should be responsible for deleting it. If you had code
like the following, you would have never had any doubt about what to
delete and what not:

iconView->insert_item (..., ..., ...);

Here, 'insert_item' would create the new item and thus is (as it
should be) also responsible for deleting it.

regards
 
I

Ian Collins

Hello!

I was looking at the following piece of code:

void MyClass::myMethod( )
{
MyItem *item = NULL;


item = new MyItem( ..., iconView, ..., ...);
}

I thought it was necessary to delete the item before the end of
myMethod scope to avoid
memory leak.
It is, MyItem should be deleted. Cases like this are ideal candidates
for std::auto_ptr.
Then I looked at the MyItem constructor:

class MyItem : public MyIconViewItem
{
public:
MyItem(..., MyIconView *parent, ..., ...);
}

Documentation for MyIconViewItem says:
When the MyIconView is deleted, all items in it are deleted
automatically.

Can I assume that the memory above is deallocated when MyIconView is
deleted?
How can it, if item was a local varable in the method MyClass::myMethod()?
 
N

n2xssvv g02gfr12930

Hello!

I was looking at the following piece of code:

void MyClass::myMethod( )
{
MyItem *item = NULL;


item = new MyItem( ..., iconView, ..., ...);
}

I thought it was necessary to delete the item before the end of
myMethod scope to avoid
memory leak.

Then I looked at the MyItem constructor:

class MyItem : public MyIconViewItem
{
public:
MyItem(..., MyIconView *parent, ..., ...);
}

Documentation for MyIconViewItem says:
When the MyIconView is deleted, all items in it are deleted
automatically.

Can I assume that the memory above is deallocated when MyIconView is
deleted?


Best regards.
Gianpaolo
If an object is created with new within any scope and not freed up with
delete before exiting that scope then yes you will leak memory.
More esoteric problems occur when multiple objects access some common
allocated object and it frees up before all the objects have no need to
access it. Hence garbage collection is such a difficult area in C++,
requiring a detailed and accurate understanding of code to avoid problems.

JB
 
G

gianpaolof

Hi JB,
that's exactly what I've always thought: if there's a "new" somewhere
within a scope,
there has to be a "delete". But we had a discussion here, after reading
the documentation, and I finally decided to ask to the gurus.

Thanks everybody for answering me!

ciao
 
J

Jakob Bieling

that's exactly what I've always thought: if there's a "new" somewhere
within a scope,
there has to be a "delete". But we had a discussion here, after

It is not true, tho. Take this piece of code (which I consider bad
practice, as I pointed out in my first reply):

class MyItem;

class MyIconView
{
public:
std::vector <MyItem*> items;

~MyIconView ();
};

class MyItem
{
public:
MyItem (MyIconView *parent)
{
parent->items.push_back (this);
}
};



MyIconView::~MyIconView ()
{
for (std::vector <MyItem*>::iterator i = items.begin (); i !=
items.end (); ++ i)
{
delete *i;
}
}



class MyClass
{
public:

MyIconView* iconView;

MyClass ()
{
iconView= new MyIconView;
}

~MyClass ()
{
delete iconView;
}

void myMethod ();

};

void MyClass::myMethod ()
{
MyItem* item = 0;

// ...

item = new MyItem (iconView);
}

int main ()
{
MyClass mc;

mc.myMethod ();
}


In the above code, you do *not* have any memory leak at all, but it
resembles your code very closely.

regards
 
J

Jakob Bieling

Jakob Bieling said:
(e-mail address removed) wrote:
It is not true, tho.

Allow me to rephrase :) It is not true that you have to delete in
the *same* scope. Of course, there is no question about having exactly
one delete for each new. But as you can see in the little example I put
together, this delete can be far away from your code and obviously is,
in your case.

I agree that it is unclear from the documentation whether it is
truely deleted correctly, which is why I consider it a very bad design.
But in general, this is not a memory leak. You need to check the code
you have *not* posted, if the item gets deleted or not.

regards
 
G

gianpaolof

Jakob said:
I agree that it is unclear from the documentation whether it is
truely deleted correctly, which is why I consider it a very bad design.
But in general, this is not a memory leak. You need to check the code
you have *not* posted, if the item gets deleted or not.

I checked the code carefully (the method body is not so complicated),
there's no evidence of a direct delete.

By the way, I did not expect Qt library to have such a design.

ciao :)
 
B

Bob Hairgrove

How can it, if item was a local varable in the method MyClass::myMethod()?

The constructor will most likely pass the "this" pointer to its
parent. According to the documentation, MyIconView will delete its
items.

Not the best design, of course, but it is unfortunately often done
that way. Ideally, the item should be clonable so that what the parent
has to delete is transparent to the creators of local items. Then item
could be created locally instead of with "new".
 
N

n2xssvv g02gfr12930

Jakob said:
It is not true, tho. Take this piece of code (which I consider bad
practice, as I pointed out in my first reply):

class MyItem;

class MyIconView
{
public:
std::vector <MyItem*> items;

~MyIconView ();
};

class MyItem
{
public:
MyItem (MyIconView *parent)
{
parent->items.push_back (this);
}
};



MyIconView::~MyIconView ()
{
for (std::vector <MyItem*>::iterator i = items.begin (); i !=
items.end (); ++ i)
{
delete *i;
}
}



class MyClass
{
public:

MyIconView* iconView;

MyClass ()
{
iconView= new MyIconView;
}

~MyClass ()
{
delete iconView;
}

void myMethod ();

};

void MyClass::myMethod ()
{
MyItem* item = 0;

// ...

item = new MyItem (iconView);
}

int main ()
{
MyClass mc;

mc.myMethod ();
}


In the above code, you do *not* have any memory leak at all, but it
resembles your code very closely.

regards
Agreed, but to use 2 different scopes in this manner is not just bad
practice, IMHO it's completely insane. Unless of course you're
determined to write code that is next to impossible to understand,
develop or maintain. Hence the following:

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

WARNING : The above memory management technique should be avoided.

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

JB
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads


Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,483
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top