is this a memory leak?

Discussion in 'C++' started by gianpaolof@gmail.com, Mar 29, 2006.

  1. Guest

    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
    , Mar 29, 2006
    #1
    1. Advertising

  2. 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)
    Jakob Bieling, Mar 29, 2006
    #2
    1. Advertising

  3. Ian Collins Guest

    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.
    Ian Collins, Mar 29, 2006
    #3
  4. 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
    n2xssvv g02gfr12930, Mar 29, 2006
    #4
  5. Guest

    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
    , Mar 29, 2006
    #5
  6. 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, Mar 29, 2006
    #6
  7. Jakob Bieling <> wrote:

    > 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)
    Jakob Bieling, Mar 29, 2006
    #7
  8. Guest

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

    regards
    , Mar 29, 2006
    #8
  9. Guest

    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 :)
    , Mar 29, 2006
    #9
  10. On Wed, 29 Mar 2006 20:37:37 +1200, Ian Collins <>
    wrote:

    > 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
    Bob Hairgrove, Mar 29, 2006
    #10
  11. Jakob Bieling wrote:
    > 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
    n2xssvv g02gfr12930, Mar 29, 2006
    #11
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. =?Utf-8?B?Y3liZXJzdHJpa2U=?=

    datagrid memory leak?

    =?Utf-8?B?Y3liZXJzdHJpa2U=?=, Jan 3, 2005, in forum: ASP .Net
    Replies:
    0
    Views:
    450
    =?Utf-8?B?Y3liZXJzdHJpa2U=?=
    Jan 3, 2005
  2. s.subbarayan

    Dynamic memory allocation and memory leak...

    s.subbarayan, Mar 18, 2005, in forum: C Programming
    Replies:
    10
    Views:
    675
    Eric Sosman
    Mar 22, 2005
  3. Richard Heathfield

    Leak or no leak ??

    Richard Heathfield, Jul 10, 2006, in forum: C Programming
    Replies:
    4
    Views:
    337
    Richard Heathfield
    Jul 10, 2006
  4. cham
    Replies:
    5
    Views:
    752
  5. Mark Probert
    Replies:
    4
    Views:
    315
    Mark Probert
    Feb 9, 2005
Loading...

Share This Page