Storing objects in vectors - what's wrong with this code?

[Alfonso Morra]

I have written the following code, to test the concept of storing
objects in a vector. I encounter two run time errors:

1). myClass gets destructed when pushed onto the vector
2). Prog throws a "SEGV" when run (presumably - attempt to delete
deleted memory.

Please take a look and see if you can notice any mistakes I'm making.
Basically, I want to store classes of my objects in a vector. I also
have three further questions:

1). What happens to my objects when erase() or clear() is invoked on the
container vector?

2). What is the best way to create new objects for pushing on to the
vector (the new keyword returns a pointer to the class - which suggests
I should probably be storing *pointers* to the class rather than the
class itself? - but push_back() has signature push_back(const T&) - I'm
confused)

3). Do I have to manually handle destruction of my objects when:

(a) pop_back is invoked on the vector (in this case what happens to my
last element?)
(b). The vector is resized (who frees my objects at the end?)
(c). The vector goes out of scope and its destructor is called?


Here is the code snippet mentioned previously:

#include <vector>
#include <iostream>

using namespace std ;

class MyClass {
public:
MyClass(){
cout << "cstor called !" << endl ;
i = new int ;
*i = 100 ;
}

virtual ~MyClass(){
cout << "dstor called !" << endl ;
delete i ;
}

int getVal(void){ return *i ; }
void setVal(int v){ *i=v ;}

private:
int* i ;
};

int main(int argc, char* argv[])
{
MyClass mc ;
std::vector<MyClass> mv ;

mv.push_back(mc) ; // Crashes here. Also I want to store
// a dynamic number of objects in the
// vector. How may I do this?
mv.clear ;
return 0;
}

 

[benben]

I have written the following code, to test the concept of storing

objects in a vector. I encounter two run time errors:

1). myClass gets destructed when pushed onto the vector
2). Prog throws a "SEGV" when run (presumably - attempt to delete
deleted memory.

Please take a look and see if you can notice any mistakes I'm making.
Basically, I want to store classes of my objects in a vector. I also
have three further questions:

1). What happens to my objects when erase() or clear() is invoked on the
container vector?

2). What is the best way to create new objects for pushing on to the
vector (the new keyword returns a pointer to the class - which suggests
I should probably be storing *pointers* to the class rather than the
class itself? - but push_back() has signature push_back(const T&) - I'm
confused)

3). Do I have to manually handle destruction of my objects when:

(a) pop_back is invoked on the vector (in this case what happens to my
last element?)
(b). The vector is resized (who frees my objects at the end?)
(c). The vector goes out of scope and its destructor is called?


Here is the code snippet mentioned previously:

#include <vector>
#include <iostream>

using namespace std ;

class MyClass {
public:
MyClass(){
cout << "cstor called !" << endl ;
i = new int ;
*i = 100 ;
}

virtual ~MyClass(){
cout << "dstor called !" << endl ;
delete i ;
}

int getVal(void){ return *i ; }
void setVal(int v){ *i=v ;}

private:
int* i ;
};

int main(int argc, char* argv[])
{
MyClass mc ;
std::vector<MyClass> mv ;

mv.push_back(mc) ; // Crashes here. Also I want to store
// a dynamic number of objects in the
// vector. How may I do this?
mv.clear ;
return 0;
}

You need to provide a copy constructor. A default copy constructor is used
so a copied MyClass will share the same inner i, which, when deleted, will
cause problems.

ben

 

[pktiwary]

I have written the following code, to test the concept of storing
objects in a vector. I encounter two run time errors:

1). myClass gets destructed when pushed onto the vector
2). Prog throws a "SEGV" when run (presumably - attempt to delete
deleted memory.

Please take a look and see if you can notice any mistakes I'm making.
Basically, I want to store classes of my objects in a vector. I also
have three further questions:

1). What happens to my objects when erase() or clear() is invoked on the
container vector?

Since what you have in a vector is a copy of your elements, calling
erase() or clear() in a vector does not have any effects on your
variables (which are obviously not vector elements but a copy of it).

2). What is the best way to create new objects for pushing on to the
vector (the new keyword returns a pointer to the class - which suggests
I should probably be storing *pointers* to the class rather than the
class itself? - but push_back() has signature push_back(const T&) - I'm
confused)

No

std::vector<MyClass> mv ;

defines a vector mv which can accomodate 'MyClass' objects, and what
you are pushing in the vactor is mc which is a MyClass object, which is
absolutely fine. So, far so good.

3). Do I have to manually handle destruction of my objects when:

(a) pop_back is invoked on the vector (in this case what happens to my
last element?)

pop_back() returns a copy of the last element from a vector, the
original copy from the vector has been deleted. So, all the local
objects will automatically get deleted. I don't see anywhere new
(outside class MyClass), so no question of manually destructing
objects.

(b). The vector is resized (who frees my objects at the end?)

When the vector gets resized is the the vectors job to efficiently move
elements and destroy any temporaraies created during this job, so you
are not the one who really needs to worry about it, unless you are
writing a vector ofcourse.

(c). The vector goes out of scope and its destructor is called?

When a vector goes out of scope, its destructor calls destructors for
all of its elements, so all the cleanup is supposed to be done by the
vector.


You have problem somewhere else. See my comments below:

Here is the code snippet mentioned previously:

#include <vector>
#include <iostream>

using namespace std ;

class MyClass {
public:
MyClass(){
cout << "cstor called !" << endl ;
i = new int ;
*i = 100 ;
}

virtual ~MyClass(){
cout << "dstor called !" << endl ;
delete i ;
}

If you think, you need any one of copy constructor, assignment operator
and destructor, you need all of these.

Define a copy constructor and an assignment operator
MyClass ( const MyClass& m ) {
i = new int;
*i = m.getVal();
}
MyClass& operator= ( const MyClass& m ) {
i = new int;
*i = m.getVal();
return *this;
}

int getVal(void){ return *i ; }

Define getVal() before the copy constructor as it uses it.

void setVal(int v){ *i=v ;}

private:
int* i ;

Your class has a char*, you need a suitable copy constructor.

};

int main(int argc, char* argv[])
{
MyClass mc ;
std::vector<MyClass> mv ;

mv.push_back(mc) ; // Crashes here. Also I want to store
// a dynamic number of objects in the
// vector. How may I do this?
mv.clear ;
return 0;
}

Since there was not a copy constructor, the default copy constructor
copies the pointers while copying i. So, after the copy, both objects
point to the same memory location created in constructor using new.
This is bad, because when one gets out of scope, its destructor gets
called, which frees up to allocated memory. The otherone's i is still
pointing to the same memory this is a disaster.

/P

 

[Alfonso Morra]

Since what you have in a vector is a copy of your elements, calling
erase() or clear() in a vector does not have any effects on your
variables (which are obviously not vector elements but a copy of it).

You seem to misunderstand my question. Granted, the objects in the
vector are copies of the originals. My question was that what happens to
the objects *in the vector container* when a clear() or erase() is
called - do they get freed?

On a slightly different note - since a vector stores a copy of the item
to be stored, it behooves me to delete the original copy(ies) , since I
do not want two copies, floating about - maybe it would be better to
store pointers to my object instead?. It seems quite
inefficient/expensive of going through the steps of:

i). Object creation
ii). Making copy of the object
iii). Deleting the original object (so only the copy in the vector is kept)

It just dosen't seem right ...

No

std::vector<MyClass> mv ;

defines a vector mv which can accomodate 'MyClass' objects, and what
you are pushing in the vactor is mc which is a MyClass object, which is
absolutely fine. So, far so good.




pop_back() returns a copy of the last element from a vector, the
original copy from the vector has been deleted. So, all the local
objects will automatically get deleted. I don't see anywhere new
(outside class MyClass), so no question of manually destructing
objects.




When the vector gets resized is the the vectors job to efficiently move
elements and destroy any temporaraies created during this job, so you
are not the one who really needs to worry about it, unless you are
writing a vector ofcourse.




When a vector goes out of scope, its destructor calls destructors for
all of its elements, so all the cleanup is supposed to be done by the
vector.


You have problem somewhere else. See my comments below:

Thanks for the correction and pointers

 

[=?iso-8859-1?q?Stephan_Br=F6nnimann?=]

Remember the rule of 3:
If there's a need to define the destructor of a class,
you'll also need to provide the copy constructor
and the assignment operator.

regards, Stephan

 

[Ben Pope]

You seem to misunderstand my question. Granted, the objects in the
vector are copies of the originals. My question was that what happens to
the objects *in the vector container* when a clear() or erase() is
called - do they get freed?

I think that would probably depend on the allocator of the vector, but generally, I suspect the destructor is called, and the memory freed.

On a slightly different note - since a vector stores a copy of the item
to be stored, it behooves me to delete the original copy(ies) , since I
do not want two copies, floating about - maybe it would be better to
store pointers to my object instead?. It seems quite
inefficient/expensive of going through the steps of:

i). Object creation
ii). Making copy of the object
iii). Deleting the original object (so only the copy in the vector is kept)

It just dosen't seem right ...

1) You could let the original go out of scope... depending on what you do with this, I suspect the compiler will optimise the copy.

2) You could create the original directly in the vector:

mv.push_back(MyClass());

//Although on VS .NET you still get 1 construction, presumably two copies and 3 destructions whether you use the temporory or not.


MyClass is a type
MyClass* is also a completely valid, different type.

You can do:

std::vector<MyClass*> mv;

If you like, but it means keeping track of your pointers, so don't do this:
mv.push_back(new MyClass());

And definately don't try to use an auto_ptr to manage it.



Also, I fail to see the point of having your int member of MyClass a pointer, whats wrong with:

class MyClass {
public:
MyClass() : i(100) {
cout << "cstor called !" << endl ;
}

virtual ~MyClass(){
cout << "dstor called !" << endl ;
}

int getVal(void){ return i ; }
void setVal(int v){ i=v ;}

private:
int i;
};

In that case the compiler provided copy and assignment will be fine.

Ben

 

[Jonathan Mcdougall]

I have written the following code, to test the concept of storing
objects in a vector. I encounter two run time errors:

1). myClass gets destructed when pushed onto the vector

MyClass is a class, not an object. It cannot be destructed. In the code
below, nothing is supposed to get destroyed on the line

mv.push_back(mc) ;

2). Prog throws a "SEGV" when run (presumably - attempt to delete
deleted memory.

Yes, that's not surprising.

Please take a look and see if you can notice any mistakes I'm making.
Basically, I want to store classes of my objects in a vector. I also

No, you want to store MyClass objects in the vector. Be careful with
names!

have three further questions:

1). What happens to my objects when erase() or clear() is invoked on the
container vector?

Depends on what you mean by 'my objects'. A std::vector manages its
memory and its elements. For example:

std::vector<int> v;
for (int i=0; i<10; ++i)
v.push_back(i);

Here, when the vector goes out of scope, it will manage its ten ints.
That means you don't have to worry about it. A vector will usually be
implemented with a dynamic array, which means it will delete the its
memory with delete[], which will call the destructors for all the
objects.

Now, it does the same thing for pointers. The problem with pointers is
that they usually point to something which must be manually deleted. So
the vector will destroy all the pointers correctly, but not the objects
pointed at. You'll have to do this manually.

int main()
{
std::vector<int*> v;
for (int i=0; i<10; ++i)
v.push_back(new int(i));

// v will leak the ints if you dont do that:
for (int i=0; i<10; ++i)
delete v;

// but v will never leak the pointers you pushed
}

2). What is the best way to create new objects for pushing on to the
vector

Depends. If you don't want copies to be made (for example, if the
objects are really big to copy, or if the identity of each object is
important), you'll want to store pointers. Since you are storing
pointers, you will need the object pointed at to last longer than
usual: you'll need dynamic memory.

If you can make copy as you wish, put objects by value in the
containers. It may seem 'inefficient', but it will save you a lot of
trouble.

(the new keyword returns a pointer to the class -

The new keyword return a pointer to a newly allocated object.

which suggests
I should probably be storing *pointers* to the class rather than the
class itself? -

If you want to use dynamic memory, yes, you'll have to store pointers
to the 'objects' rather than the 'objects' themselves. But I don't
think you need dynamic memory, certainly not for the example code
below.

but push_back() has signature push_back(const T&) - I'm
confused)

Well the signature is alright if 'T' is 'MyClass*'.

std::vector<int> v;

With this, push_back becomes

push_back(const int &);

and with

std::vector<int*> v;

push_back becomes:

push_back(const int* &);

And a reference to a pointer is like an ordinary pointer (for
simplicity). So whether you have objects or pointers in your vector,
the member functions "adapt" themselves. Actually, that's the essence
of templates.

3). Do I have to manually handle destruction of my objects when:

(a) pop_back is invoked on the vector (in this case what happens to my
last element?)

Again, it depends. The 'object' itself will be destroyed. If the object
is actually a 'pointer', and that pointer points to something you
allocated yourself, you will need to destroy that 'something'.

std::vector<int*> v;

v.push_back(new int(10));

// here, 'v' has a pointer

v.push_back();

// here, 'v' destroyed its pointer, but *not*
// what it pointed at: you just leaked the
// new int

I should have done:

std::vector<int*> v;

v.push_back(new int(10));

// here, 'v' has a pointer

delete v[0];
v.push_back();

// ok!

(b). The vector is resized (who frees my objects at the end?)

WHen a vector is resized, it will never shrink, so that's not a
problem, really.

(c). The vector goes out of scope and its destructor is called?

This is equivalent to a serie of pop_back, so all the discussion above
apply. Again, the vector owns its memory, so it will destroy all the
objects it owns and it will destroy all the memory it owns. If these
objects were pointers, you must destroy the objects they point to
manually.

Here is the code snippet mentioned previously:

#include <vector>
#include <iostream>

using namespace std ;

Not recommended at file scope.

class MyClass {
public:
MyClass(){
cout << "cstor called !" << endl ;

The abbreviation is usually 'ctor'.

i = new int ;
*i = 100 ;
}

Stop.

Your class manages memory. With you have a class which manages memory,
you must remember to rule of three: copy-contructor, assignement
operator and destructor. You have got 1/3, which is 33%. You fail

The problem is when you copy a MyClass. Since you have not specified
any copy contructor or assignement operator, the compiler will generate
them. It will copy the pointer (not not the object pointed at).

MyClass m1, m2;
m1 = m2;

Here, m1 and m2 will have their pointer point to the same int in
memory. What's more, you just leaked the int of m1. When m2 will get
out of scope, it will delete its int and then m1 will the delete the
same one. Crash.

"Now I don't make any copies" you say. Well unfortunately, yes, you do.
std::vector, when you insert an object in it, will copy the object. So
std::vector first destroy an object, which deletes an int, and then
your 'mc' object goes out of scope and deletes the same one. Bang.

MyClass(const MyClass &m)
: i(new int(m.i))
{
}

MyClass &operator=(const MyClass &m)
{
delete i;
i = new int(m.i);

return this;
}

virtual ~MyClass(){
cout << "dstor called !" << endl ;
delete i ;
}

int getVal(void){ return *i ; }
void setVal(int v){ *i=v ;}

private:
int* i ;
};

int main(int argc, char* argv[])
{
MyClass mc ;
std::vector<MyClass> mv ;

mv.push_back(mc) ; // Crashes here. Also I want to store
// a dynamic number of objects in the
// vector. How may I do this?

std::vector is *meant* to store a dynamic number of objects. Just call
push_back(), insert(), pop_back(), clear(), erase() or whatever as many
times as you want.

mv.clear ;

This should be

mv.clear();

but is not necessary anyways.

return 0;
}

Jonathan