is this a memory leak?

[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

[Jakob Bieling]

gi********@gmail.com wrote:

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
--
jb

(reply address in rot13, unscramble first)

[Ian Collins]

gi********@gmail.com wrote:

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()?

--
Ian Collins.

[n2xssvv g02gfr12930]

gi********@gmail.com wrote:

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

[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

[Jakob Bieling]

gi********@gmail.com wrote:

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
--
jb

(reply address in rot13, unscramble first)

[Jakob Bieling]

Jakob Bieling <ar****************@rot13.com> wrote:

gi********@gmail.com wrote:

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.

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
--
jb

(reply address in rot13, unscramble first)

[gianpaolof]

Ok, Jacob, I see the point now.
Thanks a lot!

regards

[gianpaolof]

Jakob Bieling wrote:

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 :)

[Bob Hairgrove]

On Wed, 29 Mar 2006 20:37:37 +1200, Ian Collins <ia******@hotmail.com>
wrote:

gi********@gmail.com wrote:

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()?

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".

--
Bob Hairgrove
No**********@Home.com

[n2xssvv g02gfr12930]

Jakob Bieling wrote:

gi********@gmail.com wrote:

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

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