Need help with vector of pointers

[LivvyB]

Hi,

I'm very new to programming and have up until now created procedural models without any dynamic memory etc. Ultimately I will be creating models of individuals that will move around on a grid, breed and then die and so first have tried to play around with some very simple code to get me started but am having difficulty getting my head around it. I was aiming to store instances of a class in a vector of pointers and so have been playing around with the following code which has 2 of these vectors. In the code the class only holds 3 data members as an example, adds instances and assigns them etc. Not using iterators as yet but aim to do so.

The problem I'm having is that when I (try to!) delete an instance of the class pointed to by the vectors it only seems to delete the m_Species and m_Colour data members as it will still output m_Name and therefore create a memory leak when I clear the vector. Is this something to do with not having a copy constructor and destructor? I tried creating a destructor but couldn't get that to work. Do the data members also need to be pointers as well for any of this to work? Part of the problem is I'm not familiar with using a vector of pointers to objects with data members!

Any help is very much appreciated!
Thanks

#pragma hdrstop
#include <vector>
#include <deque>
#include <string>
#include <iostream>


using namespace std;
//---------------------------------------------------------------------------
#pragma argsused

class Animal{

public:

static int s_total;
Animal(string word="zero", string word2="monkey",int number=0);

//~Animal();
//Animal(const Animal& c); //copy constructor prototype
static int GetTotal();
string m_Species;
string m_Colour;
int m_Name;

};
vector<Animal*> Adults;
vector<Animal*> Juvs;

int Animal::s_total=0;

Animal::Animal(string word, string word2, int number):m_Colour(word),m_Species(word2),m_Name(number) //constructor
{++s_total;}

int Animal::GetTotal(){
return s_total;
}


/*Animal::~Animal(){ //destructor definition
cout<<"Destructor called\n";
//delete m_Name;
//delete m_Colour;

} */
void printAdults();
void printJuvs();



int main(int argc, char* argv[])
{


Adults.push_back(new Animal("red","bird",1));
Adults.push_back(new Animal("blue", "cat",2));
Adults.push_back(new Animal("green", "dog",3));
Adults.push_back(new Animal("yellow", "rabbit",4));

//delete Adults[2];
printAdults();


Juvs.assign(Adults.begin(),Adults.end());

printAdults();
//printJuvs();

Juvs.push_back(new Animal("red", "bird",11));
Juvs.push_back(new Animal("blue", "cat",12));
Juvs.push_back(new Animal("green","dog",13));
Juvs.push_back(new Animal("yellow","rabbit",14));

printJuvs();
Adults.clear();
for(int i=0;i<Juvs.size();i++){
if(Juvs[i]->m_Name==2||Juvs[i]->m_Name==12){
Adults.push_back(Juvs[i]);
}
else{delete Juvs[i];
--Animal::s_total;}
}
//Juvs.clear();
printAdults();

printJuvs();

cout<<"Adults size is: "<<Adults.size()<<endl;
// cout<<"Adults capacity is: "<<Adults.capacity()<<endl;
cout<<"Juvs size is: "<<Juvs.size()<<endl;
//cout<<"Juvs capacity is: "<<Juvs.capacity()<<endl;
cout<<Animal::GetTotal()<<"\n"<<endl;

//for(int i=0;i<Juvs.size();i++){
if(Adults[0]->m_Name==2){
delete Adults[0];
}
// }
printAdults();

cin.get();
return 0;
}

void printAdults (){
for(int i=0;i<Adults.size();i++){
cout<< Adults[i]<<" "<<Adults[i]->m_Name<<" "<<Adults[i]->m_Colour<<" "<<Adults[i]->m_Species<<endl;
}
cout<<"\n"<<endl;
}


void printJuvs(){
for(int i=0;i<Juvs.size();i++){
cout<<Juvs[i]<<" "<<Juvs[i]->m_Name<<" "<<Juvs[i]->m_Colour<<" "<<Juvs[i]->m_Species<<endl;
}
cout<<"\n"<<endl;
}

[monarch_dodra]

Double Post, sorry

[monarch_dodra]

The problem is you are calling delete on a MEMBER of the vector. It will delete the object POINTED BY the vector, but not remove the actual pointer from your vector.

What you have is a dangling pointer. It is a pointer that points to un-allocated memory. When you access it, anything can happen (undefined behavior). What usually happens is that the memory will keep the old values for a while, but then, it will change, you you will start to have god awful behaviors.

You have two choices:
1- Set your pointer to zero after deletion, so you know it points to nothing, and skip it.
2- Remove the pointer from the vector after deletion.

Does that help?

PS, why use a vector of pointers when you can use a vector of objects? You'll have no dangers of leaks this way.

[LivvyB]

Thanks for that I wasn't sure if it could've just been that the pointer happened to be pointing to old memory. I was clearing the vector but wanted to check that the memory was being deleted from the heap. I expected the pointers to be pointing to some other random values so thanks for your help. I didn't know if what I was doing was completely freeing the object memory.

I decided to use a vector of pointers as thought this would improve efficiency when I apply it to my more complex models - in these cases there can be hundreds or even thousands of individuals of the same class and I thought this could slow it down and I have run out of memory in the past. Is this right? I was also thinking of using a deque for this purpose as the number of instances would keep increasing and then flutuate.

Does it make sense to free the memory the way I've done it or should you really always use a destructor? If so do you need members that are pointers as I can't seem to delete individual members from an object.

Thanks

[monarch_dodra]

You can only delete objects that are dynamically allocated. Look at this:

struct person
{
    int _age; //Not dynamically allocated
    std::string _name; //Not dynamically allocated
};

int main()
{
    person pPerson = new Person; //Dynamically allocated
    print(*pPerson); //Ok
    print(pPerson->_name); //Ok
    delete pPerson->_name; //NO! you cannot delete this object
    delete pPerson; //OK
    print(pPerson->_name); //The physical memory still exists, but reading it is a big nono
} 

I hope this small example helps?
Remember. Everything dynamically allocated MUST be manually deleted. "delete" calls your destructor.

Anything not dynamically allocated is automatically destroyed at the end of its "scope". No need to delete it manually.

Finally (and this is the kicker). You should NEVER delete objects on the stack, delete an object several times, read in un-allocated memory, write out of bounds etc. C++ however will allow you to do all these things, but the final program will crash in the most brain numbing ways, leading to hours of debug, often tears, and sometimes blood.

I highly suggest you read a good book about C++ memory management before trying to do it yourself.

PS. A good design in C++ is RAII. In this design, while pointers are legal, the rule of thumb is that if you are using the words "new" or "delete", than you are doing it wrong.

PS2. Look into Boost:: ptr_vector. This container will manage the lifetime of your objects for you.

[LivvyB]

Thanks for that monarch_dodra. I've been reading up quite a bit on this but I think I'm just getting a bit stuck on the best way of allocating lots of instances of the same class in a model and being able to swap and copy them between containers. Is it a bad idea to delete memory pointed to by a vector of pointers in the way I have done, and instead you should define a destructor for the object to clear up itself? Sorry, I know this is all pretty basic stuff but I haven't been able to find any examples really of a program that stores many instances of a class and then manipulates them.

Thanks for your help

[souldog]

you should be using boost::shared_ptr instead of raw pointers

http://www.boost.org/doc/libs/1_40_0...shared_ptr.htm

[monarch_dodra]

Is it a bad idea to delete memory pointed to by a vector of pointers in the way I have done, and instead you should define a destructor for the object to clear up itself?

YES!!! As a general rule, an object should always clean up after ITSELF, and not you. That is the very essence of RAII. If you understand that, you are golden.

For your problem, you can either do as souldog said and use some boost::shared_ptr, which is a pointer that cleans up after itself. That, or a boost::ptr_vector, which is essentially a vector of boost::shared_ptr, with some added interface sugar.

[LivvyB]

Thanks for the replies. I'll definitely look into the shared_ptr and ptr_vector, sounds like it could be a good idea to use.

I'm still unsure though how to create a destrcutor for a class in this kind of situation where the memory is deleted when the vector is looped through. I can see how to release the memory at this point but I can't see how this could be done using a destructor instead.

Thanks again

[Lindley]

A destructor is only required if the object contains a manually-acquired resource, for instance a bare pointer, which was allocated by the constructor or at some point during the object's lifetime with the assumption that it was owned by the object.

If all the members of your class handle their own cleanup (or are primitives which require no cleanup), then you don't need to write a destructor; the default auto-generated one will do.