Sign in to follow this  
Ubermeowmix

Creating & deleting pointers

Recommended Posts

If I create a pointer reference in a class:

private:
string* m_pName;
int* m_pHealth;

Animal::Animal(const string& name = "", const int& health = 2)
{
m_pName = new string(name);
m_pHealth = new int(health);
};

Animal::~Animal()
{
delete m_pName;
m_pName = 0;
delete m_pHealth;
m_pHealth = 0;
};

then call this from an external source:

Animal MyAnimal1("Denzel", 12);

Does the object still exist on the HEAP, or do I still need to delete it? This is the bit that confuses me, as if it still exists in memory then surely I still need to get rid of it... don't I?

Any help would be appreciated.

Share this post


Link to post
Share on other sites
[s]You simply can't do that. At the call site you created a couple of temporaries, they're only good for the scope of that function (which in this case is the constructor). By stashing those pointers into the class variables you have [b]not[/b] extended the lifetime of the temporaries, all that happens is that when the constructor returns your two member pointers are left pointing to un-allocated memory.[/s]

Edit: Whoops I didn't spot you were creating copies on the heap. Silly me. Edited by dmatter

Share this post


Link to post
Share on other sites
So that's a memory leak?!

So do I return a pointer reference to the call of Animal:
*pMyAnimal1 = Animal tempAnimal("Denzel", 12);

then only when I:
delete pMyAnimal1;
and kill dangly pointers:
pMyAnimal1 = 0;

does the memory cease to exist, leaving no floaty memory in the system anymore!

Share this post


Link to post
Share on other sites
There are three different things allocated in your example. A string, an int, and an object (MyAnimal1) of class Animal. The allocation of MyAnimal1, which consists of two pointers, depends on where the declaration is. If the declaration 'Animal MyAnimal1("Denzel", 12);' is in a function, then the object will be allocated on the stack. That also means that it will be deallocated when the function body goes out of scope. That is also when the destructor will be called.

However, if MyAnimal1 is declared as a global parameter, it will be located in the BSS area (pre allocated by the linker). It will not get deallocated until you program terminates.

For the two pointers, the situation is different. The content they point top will always be on the heap. And they will be freed when the destructor is called.

Why are you using a pointer to a string? Why not use the string itself in the class?

You don't need to assign 0 to the pointers in the destructor. The memory will be freed anyway. However, it may be a good practice, helping you to catch errors. Normally, you must not refer to an object that has been destructed, but it is possible if you have a pointer to it. The source code you show above has no such pointer, though.

Share this post


Link to post
Share on other sites
You do not have to delete MyAnimal1, it'll be allocated on the stack. The memory for the member variables are allocated on the heap, but the destructor takes care of those. In short, if you don't 'new' a variable you don't have to delete it(generally).

I don't see any reason to use a pointer to a string or integer in this scenario though, I'd change the member variables to non-pointers.

Share this post


Link to post
Share on other sites
[quote name='dmatter' timestamp='1343165488' post='4962740']
You simply can't do that. At the call site you created a couple of temporaries, they're only good for the scope of that function (which in this case is the constructor). By stashing those pointers into the class variables you have [b]not[/b] extended the lifetime of the temporaries, all that happens is that when the constructor returns [size=4][b]your two member pointers are left pointing to un-allocated memory[/b][/size].
[/quote]

This isn't right. The only thing that would be "temporary" is the object MyAnimal1, assuming it's declared in a function. In that case, MyAnimal1 will be created on the stack, but the member variables would be allocated from the heap. When your function ends, the destructor is called, and the member variables are removed from the heap. You don't need to NULL them out.

Please don't up vote something if you don't understand what's being said.

OP, I hope that answers your question.

Share this post


Link to post
Share on other sites
Thanks for all the help guys, i guess i've learned a lesson on taking advice as well as pointers :D

[quote name='larspensjo' timestamp='1343169071' post='4962763']
Why are you using a pointer to a string? Why not use the string itself in the class?
[/quote]

Just trying to get my head around what I can and can't do really, i want to be creating entities with alterable stats later. So I'm trying to understand the correct ways to load and unload them when I get to generating 100+ of them.

One more question if I can, when I do load multiple objects, whats the best method to use. Is it a vector of class objects? Or is there another standard method I'm not aware of?

Share this post


Link to post
Share on other sites
[quote name='Ubermeowmix' timestamp='1343165067' post='4962739']
Does the object still exist on the HEAP, or do I still need to delete it? This is the bit that confuses me, as if it still exists in memory then surely I still need to get rid of it... don't I?
[/quote]

MyAnimal1 is created on the stack
The pointers within the class are created on the stack.
The memory that the pointers point to is created on the heap.

Because of the way you have written you destructor all the memory will be released when it is called, so you have done everything right. Especially the redundant nulling of the pointers, its good practice, but might have performance issues in games.

However, I can't see why you are using dynamic memory for this data, why not just let the class members be a string and an int? I apreciate you may have simplified it for the question, in which case ignore this comment.

Share this post


Link to post
Share on other sites
[quote name='Ubermeowmix' timestamp='1343198152' post='4962834']
One more question if I can, when I do load multiple objects, whats the best method to use. Is it a vector of class objects? Or is there another standard method I'm not aware of?
[/quote]
It depends a little on what you want to do. There are a couple of possibilities:

Animal myAnimals[10];

This will allocate space for 10 objects. Again, on the stack if locally in side a function. All 10 objects will have the constructor executed, but with no arguments. Of course, it is better to replace '10' with MAX or something similar.

Animal *myAnimals[10];

This will allocated 10 pointers, initialized to random values. You need to allocate each Animal of your own, and assign it. For example,
myAnimals[0] = new Animal("Denzel", 12);
You will now have to make sure you deallocate each pointer eventually.

Animal *myAnimals = new Animal[10];

This will allocate one space for 10 objects. The advantage compared to the first example is that it will not be deallocated as soon as the declaration goes out of scope. The disadvantage is that you have to deallocate them yourself eventually, doing "delete[] myAnimals;".

[url="http://en.cppreference.com/w/cpp/container/vector"]std::vector[/url]<Animal> myAnimals;

This is maybe the best solution. It will define a vector of Animals, that can be expanded dynamically. It keeps track of the number of elements, which means you don't need a separate counter. It will be deallocated automatically when you go out of scope, so you will not need to do any delete on your own. The variable myAnimals is a "first class object" (compared to arrays in C++ that are handled as pointers). That means you can easily make a copy of it. There is some overhead, but it is very low, so don't worry about performance until you have measurments showing otherwise.

[url="http://en.cppreference.com/w/cpp/memory/unique_ptr"]std::unique_ptr[/url]<Animal> myAnimal;

This is a smart pointer to one object. So not really about arrays, but nice to know anyway. You can use it with the same syntax as if it was a normal pointer. It has a few interesting attributes: It will be deallocated automatically and it can't be copied elsewhere. It may look like a disadvantage, but that way you can guarantee that there is no danging pointer stashed away somewhere else when the object is deleted.

Share this post


Link to post
Share on other sites
[quote name='BeerNutts' timestamp='1343176721' post='4962786']
[quote name='dmatter' timestamp='1343165488' post='4962740']
You simply can't do that. At the call site you created a couple of temporaries, they're only good for the scope of that function (which in this case is the constructor). By stashing those pointers into the class variables you have [b]not[/b] extended the lifetime of the temporaries, all that happens is that when the constructor returns [b]your two member pointers are left pointing to un-allocated memory[/b].
[/quote]

This isn't right.[/quote]
Thanks for spotting my mistake BeerNutts! I hadn't noticed that the constructor is actually constructing copies; I misread it as a straight pointer assignment. My bad.

Share this post


Link to post
Share on other sites
That's awesome guy's I really appreciate the time you've taken to explain that.

Particularly like your input larspensjo, it's a great explanation.

Right I'm off to make more spelling mistakes and get tied up in knots lol.

Share this post


Link to post
Share on other sites
One last thing.

[quote name='FredOrJoeBloggs' timestamp='1343206959' post='4962860']
Because of the way you have written you destructor all the memory will be released when it is called, so you have done everything right. Especially the redundant nulling of the pointers, its good practice, but might have performance issues in games.
[/quote]

Oh in what sense is that a bad thing, as this is definitely games coding!

[quote name='FredOrJoeBloggs' timestamp='1343206959' post='4962860']
However, I can't see why you are using dynamic memory for this data, why not just let the class members be a string and an int? I apreciate you may have simplified it for the question, in which case ignore this comment.
[/quote]

So if i want to store floating point numbers and strings they can just be basic type def calls and that will save on CPU power will it?, what would I need to use the dynamic memory for? just the initial instantiation of the class (or Animal) into a vector (as in larspensjo's example), and then delete it if the object dies in game for instance.

Share this post


Link to post
Share on other sites
[quote name='Ubermeowmix' timestamp='1343242845' post='4963039']what would I need to use the dynamic memory for?[/quote]
To control the lifetime of objects.

When an object is created on the stack, its lifetime is bound to the scope it resides in. Similarly, the lifetime of the members inside an object is bound to the lifetime of the object. Because the lifetime of objects on the stack are predictable (LIFO ordering), allocating objects on the stack is fast. However, objects aren't always allocated and deallocated in order. The heap serves to manage dynamically allocated objects that can be allocated an deallocated in any order. By using dynamically allocated memory, you can control when and where to end an object's life.

Share this post


Link to post
Share on other sites
Thanks fastcall, upon testing the above I've hit on another 'wall'!

Using a vector creates a dynamic list of objects, and I want to kill off these objects by reducing their lifeforce muhahahaha *ahem*.
When I iterate through this list and remove the dead ones by calling the destructor to kill them off, the vector fails as the original scope of the for loop fails. How do I say check them all then delete the ones that are dead for a new list?

Share this post


Link to post
Share on other sites
The destructor is called automatically; do not manually call the destructor*.

Removing elements from a vector while iterating looks something like this:
[code]
vector<Object*> objects;
for ( vector<Object*>::iterator itr = objects.begin(); itr != objects.end(); /* empty */ ) {
if ( itr->shouldBeRemoved() ) {
delete *itr; // Release memory allocated for the object pointed by the pointer in the iterator
itr = object.erase( itr );
} else
itr ++;
}
[/code]

* Unless the object was constructed using placement-new. Edited by fastcall22

Share this post


Link to post
Share on other sites
Notice the difference between "[font=courier new,courier,monospace]vector<Object[b]*[/b]> objects;[/font]" and "[font=courier new,courier,monospace]vector<Object> objects;[/font]" (the '*').

The first is a vector of pointers to objects (where you have to do new/delete on individual objects), and the second is a vector of objects (no new/delete needed, it is all automatic). As mentioned above by fastcall22, what you use depends what requirements you have on the lifetime of the individual objects.

Notice also that std::vector is efficient when adding elements, but less efficient when removing elements that are not in the end. It is done by moving all subsequent elements "down", to overwrite the space of the removed element.

If you want to have long lists where you add and remove elements randomly, using [url="http://en.cppreference.com/w/cpp/container/list"]std::list[/url] may be better. In practice, it is implemented as a linked list. While insertion and removals are quick, it is not possible to do indexing. There are a couple of other nice containers, each optimized for a special purpose.

As the API to the container types are similar, but not identical, it means it is usually easy to start the implementation using one container type, and change it later when you find you have special performance requirements. For example, iterating over a vector as fastcall22 shows is the same syntax that is used to iterate over a std::list. And then, in C++11, there is special syntactical support for iteration statements.

Share this post


Link to post
Share on other sites
[quote name='fastcall22' timestamp='1343249696' post='4963063']
vector objects;
for ( vector::iterator itr = objects.begin(); itr != objects.end(); /* empty */ ) {
if ( itr->shouldBeRemoved() ) {
delete *itr; // Release memory allocated for the object pointed by the pointer in the iterator
itr = object.erase( itr );
} else
itr ++;
}
[/quote]

This is evaluating them all to death when only 1 of them is at zero (m_Health <= 0), I don't get why?

Share this post


Link to post
Share on other sites
[quote name='larspensjo' timestamp='1343255365' post='4963087']
If you want to have long lists where you add and remove elements randomly, using [url="http://en.cppreference.com/w/cpp/container/list"]std::list[/url] may be better. In practice, it is implemented as a linked list. While insertion and removals are quick, it is not possible to do indexing. There are a couple of other nice containers, each optimized for a special purpose.
[/quote]

This is the bit i'm trying to figure out, I wan't lots of baddies being generated and killed so i'm looking for the most efficient way to do it. If it's lists then I'll go off and research them over vectors.

What I essentially want are lots of objects all with specific data that can be called on depending on the users distance from that object. Like an interaction thing, so the calls for checking stats etc will be less the further away the user is from them.

Share this post


Link to post
Share on other sites
[quote name='Ubermeowmix' timestamp='1343255755' post='4963092']
This is the bit i'm trying to figure out, I wan't lots of baddies being generated and killed so i'm looking for the most efficient way to do it. If it's lists then I'll go off and research them over vectors.[/quote]
I'm going to put words in your mouth and say that you don't want to do that, you just want to get it working. Figuring out the most efficient way to do it is going to require you to write both methods and profile them. You only want to do that if there's an actual need for it(i.e. your game is running to slow). Stick with vectors for now.

[quote name='Ubermeowmix' timestamp='1343255439' post='4963090']
This is evaluating them all to death when only 1 of them is at zero (m_Health <= 0), I don't get why?
[/quote]
Could you paste the relevant code?

Share this post


Link to post
Share on other sites
vector<Animal> myAnimals; // globally in main

// I then fill myAnimals with a for loop that reads back perfectly and shows names and health properly.
// e.g. Animal newAnimal(animalsNames[i], 10 + i); // animalsNames is just a string array
// myAnimals.push_back(newAnimal);

AttackAnimals(2);

void AttackAnimals(int dam)
{
for(int i = 0; i < myAnimals.size(); i++)
{
myAnimals[i].SetHealth(dam);
};

for ( vector<Animal>::iterator iter = myAnimals.begin(); iter != myAnimals.end(); /* empty */ )
{
if ( iter->Check4Death() )
{
cout << iter->GetName() << " has died!\n";
iter = myAnimals.erase( iter );
} else {
iter ++;
}
}
};

// the following are the functions from the class

void Animal::SetHealth(int minus)
{
*m_pHealth -= minus;
}

bool Animal::Check4Death()
{
if(*m_pHealth <= 0)
return true;
else
return false;
}; Edited by Ubermeowmix

Share this post


Link to post
Share on other sites
[quote name='Ubermeowmix' timestamp='1343242845' post='4963039']
[quote name='FredOrJoeBloggs' timestamp='1343206959' post='4962860']
Especially the redundant nulling of the pointers, its good practice, but might have performance issues in games.
[/quote]
Oh in what sense is that a bad thing, as this is definitely games coding!
[/quote]

It depends on what your compiler does.
If it compiles the code as is, then it will waste a few instructions.
If the compiler optimises the code, it might well remove the code.
If you want speed remove redundent code before you compile.

[quote name='Ubermeowmix' timestamp='1343242845' post='4963039']
[quote name='FredOrJoeBloggs' timestamp='1343206959' post='4962860']
However, I can't see why you are using dynamic memory for this data, why not just let the class members be a string and an int? I apreciate you may have simplified it for the question, in which case ignore this comment.
[/quote]
So if i want to store floating point numbers and strings they can just be basic type def calls and that will save on CPU power will it?, what would I need to use the dynamic memory for? just the initial instantiation of the class (or Animal) into a vector (as in larspensjo's example), and then delete it if the object dies in game for instance.
[/quote]
Dynamic memory is useful if you don't know who many of the objects you are going to have. If you know how many objects of a certain type you will have at maximum AND the amount of memory that uses doesn't hinder the running of the program then use stack memory. However since you are using a std::vector you are already using dynamically allocated memory (al be it managed in a library for you). The objects that you are placing into the vector can have dynamic memory, but they could also be 'non-dynamic'.
If performance is an issue then write your code such that you can replace the way the data is stored without affecting the rest of the code and then bench mark the two options by performing so many thousand operations that will excersize the code.
[source lang="cpp"]#include <afx.h>
#include <string>
#include <vector>

class CAnimalsD
{ // Animal class using dynamic memory
public:
typedef std::vector<CAnimalsD> VECTOR;
public:
CAnimalsD(const std::string& name = "", const int& health = 2)
: m_pName(new std::string(name))
, m_pHealth(new int(health))
{}
CAnimalsD(const CAnimalsD& other)
: m_pName(new std::string(*other.m_pName))
, m_pHealth(new int(*other.m_pHealth))
{}
~CAnimalsD()
{
if (m_pName)
delete m_pName;
if (m_pHealth)
delete m_pHealth;
}

private:
std::string* m_pName;
int* m_pHealth;
};

class CAnimalsS
{ // Animal class using static memory
public:
typedef std::vector<CAnimalsS> VECTOR;
public:
CAnimalsS(const std::string& name = "", const int& health = 2)
: m_Name(name)
, m_Health(health)
{}
CAnimalsS(const CAnimalsS& other)
: m_Name(other.m_Name)
, m_Health(other.m_Health)
{}
~CAnimalsS(){}
private:
std::string m_Name;
int m_Health;
};

// Gash test program.
int main (void)
{
static const int constTestCycles = 10000;
CAnimalsD::VECTOR animalsD;
CAnimalsS::VECTOR animalsS;

const DWORD startTime = GetTickCount();
for (int i = 0; i < constTestCycles; ++i)
{
animalsS.push_back(CAnimalsS("FRED", i));
}
animalsS.clear();
const DWORD midTime = GetTickCount();
for (int i = 0; i < constTestCycles; ++i)
{
animalsD.push_back(CAnimalsD("FRED", i));
}
animalsD.clear();
const DWORD endTime = GetTickCount();

printf("dynamic test = %d\n", midTime - startTime);
printf("static test = %d\n", endTime - midTime);
printf("press enter to exit\n");
return fgetc(stdin);
}[/source]

Share this post


Link to post
Share on other sites
[source lang="cpp"]void AttackAnimals(int dam)
{
for(int i = 0; i < myCritters.size(); i++)
{
myAnimals[i].SetHealth(dam);
};[/source]

The funtion is called AttackAnimals, but your iterator ranges from 0 to the size of myCritters, shouldn't that be myAnimals( this is dangerous because you might go out of bounds if myCritters.size() > myAnimals.size() )? Also, your SetHealth() method isn't very descriptive of what it does, it doesn't actually set the health to a value, but subtracts that value from it, consider renaming it to takeDamage() or something like that.

[quote]bool Animal::Check4Death()
{
if(*m_pHealth <= 0)
return true;
else
return false;
}; [/quote]

You can reduce this to:

[quote]bool Animal::Check4Death()
{
return *m_pHealth <= 0;
}; [/quote]

Looks a lot cleaner.

Now as to your problem, your m_pHealth member is a pointer, I think that could be it. Why is this a pointer? Are you sure your animals aren't sharing this pointer?

* used quotes for code because source tags don't seem to function very well Edited by Mussi

Share this post


Link to post
Share on other sites
[quote name='Mussi' timestamp='1343306941' post='4963275']
bool Animal::Check4Death()
{
return *m_pHealth <= 0;
};
[/quote]

That is cleaner code, I didn't realize that was a legal declaration.

Share this post


Link to post
Share on other sites
[quote name='Mussi' timestamp='1343306941' post='4963275']
void AttackAnimals(int dam)
{
for(int i = 0; i < myCritters.size(); i++)
{
myAnimals[i].SetHealth(dam);
};
[/quote]
that was just a typo on my part.

You've lost me with that code FredOrJoeBloggs, it doesn't seem to work either. Not really sure about templates yet, but i'll put it aside to tinker with later.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this