ptr in collection item

Discussion in 'C++' started by tuvok, Jun 16, 2005.

  1. tuvok

    tuvok Guest

    I have an Item object which allocates memory in its ctor
    and frees it in its dtor.
    I want to add such items to a vector. But the program crashes. Why?
    How do I fix it?

    #include <vector>

    class Item
    {
    public:
    int iId;
    void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    Item(int AiId) : iId(AiId) { pMiscMem = malloc(1024); }
    ~Item() { free(pMiscMem), pMiscMem = 0; }
    };

    class MyColl
    {
    public:
    std::vector<Item> v;
    MyColl() {}
    void Add(Item* pItem) { v.push_back(*pItem); }
    };

    void test_ptr_in_vector_item()
    {
    MyColl C;
    for (int i = 0; i < 100; ++i)
    C.Add(new Item(i));
    }
    tuvok, Jun 16, 2005
    #1
    1. Advertising

  2. tuvok wrote:
    > I have an Item object which allocates memory in its ctor
    > and frees it in its dtor.
    > I want to add such items to a vector. But the program crashes. Why?
    > How do I fix it?
    >


    Read about "the Rule of Three".

    V
    Victor Bazarov, Jun 16, 2005
    #2
    1. Advertising

  3. tuvok

    Jason Heyes Guest

    Your copy constructor (automatically generated by the compiler) does not
    allocate new memory. Are you trying to use heterogeneous containers like in
    Java? You should avoid this.
    Jason Heyes, Jun 16, 2005
    #3
  4. tuvok

    tuvok Guest

    "Victor Bazarov" wrote
    > tuvok wrote:
    > > I have an Item object which allocates memory in its ctor
    > > and frees it in its dtor.
    > > I want to add such items to a vector. But the program crashes. Why?
    > > How do I fix it?
    > >

    > Read about "the Rule of Three".


    Thank you. I did and came up with the following but it still crashes :-(
    What's missing?

    class Item
    {
    public:
    int iId;
    void* pMiscMem; // will be allocd in ctor and deallocd in dtor

    Item(int AiId) : iId(AiId), pMiscMem(0)
    { pMiscMem = malloc(1024); }

    ~Item()
    { free(pMiscMem), pMiscMem = 0; }

    Item(const Item& Other)
    {
    free(pMiscMem);

    iId = Other.iId;
    pMiscMem = Other.pMiscMem;

    Other.~Item();
    }

    const Item& operator=(const Item& Other)
    {
    free(pMiscMem);

    iId = Other.iId;
    pMiscMem = Other.pMiscMem;

    Other.~Item();

    return *this;
    }
    };

    class MyColl
    {
    public:
    std::vector<Item> v;
    MyColl() {}
    void Add(Item* pItem) { v.push_back(*pItem); }
    };

    void test_ptr_in_vector_item()
    {
    MyColl C;
    for (int i = 0; i < 100; ++i)
    C.Add(new Item(i));
    }
    tuvok, Jun 16, 2005
    #4
  5. tuvok

    tuvok Guest

    "Jason Heyes" wrote
    > Your copy constructor (automatically generated by the compiler) does not
    > allocate new memory. Are you trying to use heterogeneous containers like in Java?


    I think not. Cf. code. The container shall contain objects of the same one type.
    tuvok, Jun 16, 2005
    #5
  6. tuvok

    Jason Heyes Guest

    Remove 'free(pMiscMem)' in your copy constructor and all lines containing
    'Other.~Item()'.
    Jason Heyes, Jun 16, 2005
    #6
  7. tuvok wrote:
    > "Victor Bazarov" wrote
    >>tuvok wrote:
    >>>I have an Item object which allocates memory in its ctor
    >>>and frees it in its dtor.
    >>>I want to add such items to a vector. But the program crashes. Why?
    >>>How do I fix it?
    >>>

    >>Read about "the Rule of Three".

    >
    > Thank you. I did and came up with the following but it still crashes :-(
    > What's missing?
    >
    > class Item
    > {
    > public:
    > int iId;
    > void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    >
    > Item(int AiId) : iId(AiId), pMiscMem(0)
    > { pMiscMem = malloc(1024); }


    Try using 'new' and delete[], rather than malloc() and free()?
    Why is pMiscMem "void *", what will it hold?

    >
    > ~Item()
    > { free(pMiscMem), pMiscMem = 0; }
    >
    > Item(const Item& Other)
    > {
    > free(pMiscMem);
    >
    > iId = Other.iId;


    > pMiscMem = Other.pMiscMem;



    This is a copy constructor, you should be making
    a copy of "Other" - not taking over its data.
    You should replace the above line with something
    like this:

    pMiscMem = malloc(1024);
    memcpy(pMiscMem, Other.pMiscMem, 1024);

    Otherwise you'll have 2 Item objects who's pMiscMem
    pointers point to the same memory block. When either
    of those Item objects is destructed, the memory that
    both Item.pMiscMem members point to will be deleted.
    That will leave the pMiscMem of the remaining 'Item'
    object pointing to memory that already been freed;
    causing a crash when the 2nd Item object is destructed.



    >
    > Other.~Item();


    Don't delete 'Other' in your copy constructor.
    The purpose of the copy constructor is to make a copy
    of 'Other' - and that's all.


    > }
    >
    > const Item& operator=(const Item& Other)
    > {
    > free(pMiscMem);
    >
    > iId = Other.iId;


    > pMiscMem = Other.pMiscMem;


    This is an assignmet operator, you should be making
    a copy of "Other" - not taking over its data.
    You should replace the above line with something
    like this:

    pMiscMem = malloc(1024);
    memcpy(pMiscMem, Other.pMiscMem, 1024);

    Otherwise you'll have 2 Item objects who's pMiscMem
    pointers point to the same memory block. When either
    of those Item objects is destructed, the memory that
    both Item.pMiscMem members point to will be deleted.
    That will leave the pMiscMem of the remaining 'Item'
    object pointing to memory that already been freed;
    causing a crash when the 2nd Item object is destructed.

    >
    > Other.~Item();



    Don't delete 'Other' in your assignment operator.
    The purpose of the assignment operator is to make a
    copy of the data from 'Other' in 'this' - and that's all.


    >
    > return *this;
    > }
    > };
    >
    > class MyColl
    > {
    > public:
    > std::vector<Item> v;
    > MyColl() {}
    > void Add(Item* pItem) { v.push_back(*pItem); }
    > };
    >
    > void test_ptr_in_vector_item()
    > {
    > MyColl C;
    > for (int i = 0; i < 100; ++i)
    > C.Add(new Item(i));
    > }
    >
    >


    Larry
    Larry I Smith, Jun 16, 2005
    #7
  8. tuvok

    tuvok Guest

    "Larry I Smith" wrote
    > tuvok wrote:
    > > "Victor Bazarov" wrote
    > >>tuvok wrote:
    > >>>I have an Item object which allocates memory in its ctor
    > >>>and frees it in its dtor.
    > >>>I want to add such items to a vector. But the program crashes. Why?
    > >>>How do I fix it?
    > >>>
    > >>Read about "the Rule of Three".

    > >
    > > Thank you. I did and came up with the following but it still crashes :-(
    > > What's missing?
    > >
    > > class Item
    > > {
    > > public:
    > > int iId;
    > > void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    > >
    > > Item(int AiId) : iId(AiId), pMiscMem(0)
    > > { pMiscMem = malloc(1024); }

    >
    > Try using 'new' and delete[], rather than malloc() and free()?
    > Why is pMiscMem "void *", what will it hold?


    Nothing special. Just a sample for demo purposes.

    > >
    > > ~Item()
    > > { free(pMiscMem), pMiscMem = 0; }
    > >
    > > Item(const Item& Other)
    > > {
    > > free(pMiscMem);
    > >
    > > iId = Other.iId;

    >
    > > pMiscMem = Other.pMiscMem;

    >
    >
    > This is a copy constructor, you should be making
    > a copy of "Other" - not taking over its data.
    > You should replace the above line with something
    > like this:
    >
    > pMiscMem = malloc(1024);
    > memcpy(pMiscMem, Other.pMiscMem, 1024);
    >
    > Otherwise you'll have 2 Item objects who's pMiscMem
    > pointers point to the same memory block. When either
    > of those Item objects is destructed, the memory that
    > both Item.pMiscMem members point to will be deleted.
    > That will leave the pMiscMem of the remaining 'Item'
    > object pointing to memory that already been freed;
    > causing a crash when the 2nd Item object is destructed.


    Ok, got it.
    But wouldn't it be faster if 'this' would just take the resources of 'Other'
    and clear them in 'Other'? IOW transferring the ownership from
    'Other' to 'this'. The following solution does it this way. Is it ok to do it so,
    or is there anything against it?

    class Item
    {
    public:
    int iId;
    void* pMiscMem; // will be allocd in ctor and deallocd in dtor

    Item(int AiId) : iId(AiId)
    {
    pMiscMem = malloc(1024); // just a sample for ptr usage in collection items
    }

    ~Item()
    {
    if (pMiscMem)
    free(pMiscMem);
    ClearOwnership();
    }

    void ClearOwnership()
    { // just clears the resources (but does not destroy them!)
    // intended especially for ptrs.
    // will be called from copy_ctor, assignment_operator, and dtor
    pMiscMem = 0; // setting ptrs to 0 is important
    iId = 0;
    }

    Item(const Item& Other)
    { // The object will be created (initialized) from the other one (Other),
    // Therefore the normal ctor won't be called.
    // So nothing is initialized yet!
    // The initialization from Other is the job of this copy constructor.

    // set resources of this from Other:
    iId = Other.iId;
    pMiscMem = Other.pMiscMem;

    // take ownership by clearing Other:
    ((Item*) &Other)->ClearOwnership();
    }

    const Item& operator=(const Item& Other)
    {
    // free resources of this:
    free(pMiscMem);

    // set resources of this from Other:
    iId = Other.iId;
    pMiscMem = Other.pMiscMem;

    // take ownership by clearing Other:
    ((Item*) &Other)->ClearOwnership();

    return *this;
    }
    };

    class MyColl
    {
    public:
    std::vector<Item> v;
    MyColl() {}
    void Add(Item& rItem) { v.push_back(rItem); }
    };

    void test_ptr_in_vector_item()
    {
    MyColl C;
    for (int i = 1; i <= 10000; ++i)
    {
    Item obj(i);
    C.Add(obj);
    }
    }
    tuvok, Jun 16, 2005
    #8
  9. tuvok wrote:
    > I have an Item object which allocates memory in its ctor
    > and frees it in its dtor.
    > I want to add such items to a vector. But the program crashes. Why?
    > How do I fix it?
    >
    > #include <vector>
    >
    > class Item
    > {
    > public:
    > int iId;
    > void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    > Item(int AiId) : iId(AiId) { pMiscMem = malloc(1024); }
    > ~Item() { free(pMiscMem), pMiscMem = 0; }
    > };
    >
    > class MyColl
    > {
    > public:
    > std::vector<Item> v;
    > MyColl() {}
    > void Add(Item* pItem) { v.push_back(*pItem); }
    > };
    >
    > void test_ptr_in_vector_item()
    > {
    > MyColl C;
    > for (int i = 0; i < 100; ++i)
    > C.Add(new Item(i));
    > }
    >
    >
    >


    There is a memory leak in the line:

    C.Add(new Item(i));

    You use 'new' to allocate an 'Item'.
    A POINTER to that 'Item' is passed to Add() which
    calls v.push_back(*pItem). push_back() makes a
    COPY of it's arg (a copy of "*pItem" in this case),
    then puts that copy into the vector. The original
    'Item' allocated by 'new' is never deleted.
    Here's one possible alternative:

    void test_ptr_in_vector_item()
    {
    MyColl C;
    Item * pItem;

    for (int i = 0; i < 100; ++i)
    {
    pItem = new Item(i);
    C.Add(pItem);
    delete pItem;
    }
    }

    The name 'test_ptr_in_vector()' is misleading.
    You do not put a pointer to 'Item' (an "Item *")
    in your vector; you put an actual 'Item'.

    Larry
    Larry I Smith, Jun 16, 2005
    #9
  10. tuvok

    Andre Kostur Guest

    "tuvok" <> wrote in
    news:d8r5dg$mvd$01$-online.com:

    > Ok, got it.
    > But wouldn't it be faster if 'this' would just take the resources of
    > 'Other' and clear them in 'Other'? IOW transferring the ownership from
    > 'Other' to 'this'. The following solution does it this way. Is it ok
    > to do it so, or is there anything against it?


    Uh... why aren't you simply using std::auto_ptr then? Sounds like it has
    all of the semantics that you're looking for...

    And... any pointer with a transfer of ownership semantics is unsuitable for
    putting into the standard containers (note: auto_ptr included!). You might
    want to look up something like boost::shared_ptr.
    Andre Kostur, Jun 16, 2005
    #10
  11. tuvok

    tuvok Guest

    "Larry I Smith" wrote
    > tuvok wrote:
    > > I have an Item object which allocates memory in its ctor
    > > and frees it in its dtor.
    > > I want to add such items to a vector. But the program crashes. Why?
    > > How do I fix it?
    > >

    ....
    > > void test_ptr_in_vector_item()
    > > {
    > > MyColl C;
    > > for (int i = 0; i < 100; ++i)
    > > C.Add(new Item(i));
    > > }
    > >

    >
    > There is a memory leak in the line:
    >
    > C.Add(new Item(i));
    >
    > You use 'new' to allocate an 'Item'.
    > A POINTER to that 'Item' is passed to Add() which
    > calls v.push_back(*pItem). push_back() makes a
    > COPY of it's arg (a copy of "*pItem" in this case),
    > then puts that copy into the vector. The original
    > 'Item' allocated by 'new' is never deleted.
    > Here's one possible alternative:
    >
    > void test_ptr_in_vector_item()
    > {
    > MyColl C;
    > Item * pItem;
    >
    > for (int i = 0; i < 100; ++i)
    > {
    > pItem = new Item(i);
    > C.Add(pItem);
    > delete pItem;
    > }
    > }


    Thanks. Got it.

    > The name 'test_ptr_in_vector()' is misleading.
    > You do not put a pointer to 'Item' (an "Item *")
    > in your vector; you put an actual 'Item'.


    Yeah. It better should be "test_ptr_in_collectionitem()"
    tuvok, Jun 16, 2005
    #11
  12. tuvok wrote:
    > "Larry I Smith" wrote
    >>tuvok wrote:
    >>>"Victor Bazarov" wrote
    >>>>tuvok wrote:
    >>>>>I have an Item object which allocates memory in its ctor
    >>>>>and frees it in its dtor.
    >>>>>I want to add such items to a vector. But the program crashes. Why?
    >>>>>How do I fix it?
    >>>>>
    >>>>Read about "the Rule of Three".
    >>>Thank you. I did and came up with the following but it still crashes :-(
    >>>What's missing?
    >>>
    >>>class Item
    >>> {
    >>> public:
    >>> int iId;
    >>> void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    >>>
    >>> Item(int AiId) : iId(AiId), pMiscMem(0)
    >>> { pMiscMem = malloc(1024); }

    >>Try using 'new' and delete[], rather than malloc() and free()?
    >>Why is pMiscMem "void *", what will it hold?

    >
    > Nothing special. Just a sample for demo purposes.
    >
    >>> ~Item()
    >>> { free(pMiscMem), pMiscMem = 0; }
    >>>
    >>> Item(const Item& Other)
    >>> {
    >>> free(pMiscMem);
    >>>
    >>> iId = Other.iId;
    >>> pMiscMem = Other.pMiscMem;

    >>
    >>This is a copy constructor, you should be making
    >>a copy of "Other" - not taking over its data.
    >>You should replace the above line with something
    >>like this:
    >>
    >> pMiscMem = malloc(1024);
    >> memcpy(pMiscMem, Other.pMiscMem, 1024);
    >>
    >>Otherwise you'll have 2 Item objects who's pMiscMem
    >>pointers point to the same memory block. When either
    >>of those Item objects is destructed, the memory that
    >>both Item.pMiscMem members point to will be deleted.
    >>That will leave the pMiscMem of the remaining 'Item'
    >>object pointing to memory that already been freed;
    >>causing a crash when the 2nd Item object is destructed.

    >
    > Ok, got it.
    > But wouldn't it be faster if 'this' would just take the resources of 'Other'
    > and clear them in 'Other'? IOW transferring the ownership from
    > 'Other' to 'this'. The following solution does it this way. Is it ok to do it so,
    > or is there anything against it?
    >
    > class Item
    > {
    > public:
    > int iId;
    > void* pMiscMem; // will be allocd in ctor and deallocd in dtor
    >
    > Item(int AiId) : iId(AiId)
    > {
    > pMiscMem = malloc(1024); // just a sample for ptr usage in collection items
    > }
    >
    > ~Item()
    > {
    > if (pMiscMem)
    > free(pMiscMem);
    > ClearOwnership();
    > }
    >
    > void ClearOwnership()
    > { // just clears the resources (but does not destroy them!)
    > // intended especially for ptrs.
    > // will be called from copy_ctor, assignment_operator, and dtor
    > pMiscMem = 0; // setting ptrs to 0 is important
    > iId = 0;
    > }
    >
    > Item(const Item& Other)
    > { // The object will be created (initialized) from the other one (Other),
    > // Therefore the normal ctor won't be called.
    > // So nothing is initialized yet!
    > // The initialization from Other is the job of this copy constructor.
    >
    > // set resources of this from Other:
    > iId = Other.iId;
    > pMiscMem = Other.pMiscMem;
    >
    > // take ownership by clearing Other:
    > ((Item*) &Other)->ClearOwnership();
    > }
    >
    > const Item& operator=(const Item& Other)
    > {
    > // free resources of this:
    > free(pMiscMem);
    >
    > // set resources of this from Other:
    > iId = Other.iId;
    > pMiscMem = Other.pMiscMem;
    >
    > // take ownership by clearing Other:
    > ((Item*) &Other)->ClearOwnership();
    >
    > return *this;
    > }
    > };
    >
    > class MyColl
    > {
    > public:
    > std::vector<Item> v;
    > MyColl() {}
    > void Add(Item& rItem) { v.push_back(rItem); }
    > };
    >
    > void test_ptr_in_vector_item()
    > {
    > MyColl C;
    > for (int i = 1; i <= 10000; ++i)
    > {
    > Item obj(i);
    > C.Add(obj);
    > }
    > }
    >
    >


    Well, now you've changed the code considerably, and
    induced additional (new) problems - like storing
    references in a container, modifying 'const' objects,
    etc, etc.

    Exactly what is your real goal?

    We can modify 'made up' play code all day long,
    yet never answer your real question(s).

    So again, what is the actual design requirement that you
    are struggling to resolve?

    Larry
    Larry I Smith, Jun 16, 2005
    #12
  13. tuvok

    tuvok Guest

    "Larry I Smith" wrote
    > tuvok wrote:

    ....
    > Well, now you've changed the code considerably, and
    > induced additional (new) problems - like storing
    > references in a container, modifying 'const' objects,
    > etc, etc.


    The storage of items via ref has not changed. Ie. objects
    are stored as objects, not as pointers. This hasn't changed.

    > Exactly what is your real goal?


    It was just the problem of safely adding "items with non-empty
    pointer resources" to STL containers like vector.
    IMO it is solved now by the use of a copy ctor.
    Thanks everybody.
    tuvok, Jun 16, 2005
    #13
  14. tuvok

    Swampmonster Guest

    > It was just the problem of safely adding "items with non-empty
    > pointer resources" to STL containers like vector.
    > IMO it is solved now by the use of a copy ctor.
    > Thanks everybody.


    NO. Read about why you can't put std::auto_ptr into STL containers.
    The "transfer ownership" thing does not work if you store objects in
    an STL container, since the container _does not_ guarantee to copy
    the objects in a specific order so that the last-assigned-to object
    is the one that is kept. You could end up with a container full of
    empty ("cleared") objects.

    And one other thing:

    > const Item& operator=(const Item& Other)
    > {
    > // free resources of this:
    > free(pMiscMem);
    >
    > // set resources of this from Other:
    > iId = Other.iId;
    > pMiscMem = Other.pMiscMem;
    >
    > // take ownership by clearing Other:
    > ((Item*) &Other)->ClearOwnership();
    >
    > return *this;
    > }


    It might be wise to protect "operator =" against self assignment.
    An STL container might well decide assign "x=x" - the above
    code would screw up in that situation.
    Swampmonster, Jun 17, 2005
    #14
    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. Sid
    Replies:
    5
    Views:
    1,065
  2. Heiko Vogel
    Replies:
    3
    Views:
    548
    Method Man
    Sep 14, 2004
  3. franco ziade

    const ptr to const ptr ?

    franco ziade, Feb 17, 2005, in forum: C Programming
    Replies:
    3
    Views:
    387
    franco ziade
    Feb 17, 2005
  4. G Fernandes
    Replies:
    9
    Views:
    575
    DHOLLINGSWORTH2
    Feb 27, 2005
  5. Øyvind Isaksen
    Replies:
    1
    Views:
    948
    Øyvind Isaksen
    May 18, 2007
Loading...

Share This Page