ptr in collection item

T

tuvok

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));
}
 
V

Victor Bazarov

tuvok said:
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?
Click to expand...

Read about "the Rule of Three".

V
 
J

Jason Heyes

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

tuvok

Read about "the Rule of Three".
Click to expand...

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));
}
 
T

tuvok

Your copy constructor (automatically generated by the compiler) does not
allocate new memory. Are you trying to use heterogeneous containers like in Java?
Click to expand...

I think not. Cf. code. The container shall contain objects of the same one type.
 
J

Jason Heyes

Remove 'free(pMiscMem)' in your copy constructor and all lines containing
'Other.~Item()'.
 
L

Larry I Smith

tuvok said:
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); }
Click to expand...

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;
Click to expand...
pMiscMem = Other.pMiscMem;
Click to expand...


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();
Click to expand...

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;
Click to expand...
pMiscMem = Other.pMiscMem;
Click to expand...

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();
Click to expand...


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));
}
Click to expand...

Larry
 
T

tuvok

tuvok said:
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); }
Click to expand...

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

Nothing special. Just a sample for demo purposes.
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.
Click to expand...

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);
}
}
 
L

Larry I Smith

tuvok said:
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));
}
Click to expand...

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
 
A

Andre Kostur

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?
Click to expand...

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

tuvok

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;
}
}
Click to expand...

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'.
Click to expand...

Yeah. It better should be "test_ptr_in_collectionitem()"
 
L

Larry I Smith

tuvok said:
tuvok said:
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); }
Click to expand...
Try using 'new' and delete[], rather than malloc() and free()?
Why is pMiscMem "void *", what will it hold?
Click to expand...

Nothing special. Just a sample for demo purposes.
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.
Click to expand...

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);
}
}
Click to expand...

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
 
T

tuvok

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.
Click to expand...

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?
Click to expand...

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

Swampmonster

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.
Click to expand...

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;
}
Click to expand...

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.
 

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

Save instance when rotating screen 1
Save instance when rotating 0
How do I make this craftinfsystem Work 1
deleting dynamically allocated objects in a container 17
hash map / collection choice 14
Base ptr to access the derived members 4
Somone's SO question: "Is there an existing library for dynamically-determineddimensional array in c 1
Help in hangman game 1

Members online

No members online now.
Total: 27 (members: 1, guests: 26)
Robots: 548

Forum statistics

Threads
473,769
Messages
2,569,581
Members
45,056
Latest member
GlycogenSupporthealth

Latest Threads

Top