Jump to content

  • Log In with Google      Sign In   
  • Create Account


Creating & deleting pointers


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
30 replies to this topic

#21 Ubermeowmix   Members   -  Reputation: 307

Like
0Likes
Like

Posted 26 July 2012 - 12:03 AM

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, 26 July 2012 - 11:04 AM.

If you get near a point, make it!

Sponsor:

#22 FredOrJoeBloggs   Members   -  Reputation: 117

Like
0Likes
Like

Posted 26 July 2012 - 04:38 AM


Especially the redundant nulling of the pointers, its good practice, but might have performance issues in games.

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


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.


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.

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.

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 memorypublic: 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 memorypublic: 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]

#23 Mussi   Crossbones+   -  Reputation: 1580

Like
0Likes
Like

Posted 26 July 2012 - 06:49 AM

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

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


You can reduce this to:

bool Animal::Check4Death()
{
return *m_pHealth <= 0;
};


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, 26 July 2012 - 06:52 AM.


#24 Ubermeowmix   Members   -  Reputation: 307

Like
0Likes
Like

Posted 26 July 2012 - 11:03 AM

bool Animal::Check4Death()
{
return *m_pHealth <= 0;
};


That is cleaner code, I didn't realize that was a legal declaration.
If you get near a point, make it!

#25 Ubermeowmix   Members   -  Reputation: 307

Like
0Likes
Like

Posted 26 July 2012 - 12:23 PM

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

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.
If you get near a point, make it!

#26 Ubermeowmix   Members   -  Reputation: 307

Like
0Likes
Like

Posted 26 July 2012 - 02:53 PM

Oh I've figured out why the following was evaluating to all die.

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 ++;
}
}
};

it's because the constructor call was creating pointers to the member functions and killing the code, now i've swapped it all back to being normal variables the code it working fully. Thanks for all the help guys, the answers were in there. They were just dotted around a bit.
If you get near a point, make it!

#27 iMalc   Crossbones+   -  Reputation: 2214

Like
0Likes
Like

Posted 27 July 2012 - 12:49 AM

You don't need to NULL them out.

Indeed, and to get this point across, NULLing out the pointers in the destructor after freeing the memory they pointed to is like using white-out over your text on a piece of paper just before you put it through a shredder.
If you're about to shred it, then there's no point in using white-out.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

#28 larspensjo   Members   -  Reputation: 1526

Like
0Likes
Like

Posted 27 July 2012 - 01:06 AM

If you're about to shred it, then there's no point in using white-out.


I use this technique now and then. In principle, you are perfectly correct. In practice, this technique helps you find cases where you are using dangling pointers. I tend to make those mistakes myself less and less with the years (though it still happens), but for beginners I definitely recommend it. Yes, it costs some performance, but the usual rule applies: don't optimize until you have measurements.
Current project: Ephenation.
Sharing OpenGL experiences: http://ephenationopengl.blogspot.com/

#29 dmatter   Crossbones+   -  Reputation: 2728

Like
1Likes
Like

Posted 27 July 2012 - 03:38 PM

In principle, you are perfectly correct. In practice, this technique helps you find cases where you are using dangling pointers.

Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided. I feel it's better to abide by that rule than to bother nulling pointers after delete, it'll save you from more more bugs overall.

#30 larspensjo   Members   -  Reputation: 1526

Like
0Likes
Like

Posted 27 July 2012 - 04:38 PM

Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided.

I agree, of course. Maybe I wasn't clear in my description, I don't advocate it as a "design pattern". What I meant is that it helps you find bugs, while not being something you should rely on.
Current project: Ephenation.
Sharing OpenGL experiences: http://ephenationopengl.blogspot.com/

#31 Brother Bob   Moderators   -  Reputation: 7422

Like
2Likes
Like

Posted 27 July 2012 - 04:52 PM


Not sure I buy that argument really, how would you manage to use the pointers after the destructor? It's only really relevant if you call delete on a pointer where subsequent code still has access to that pointer - such a scenario, whilst easy to concoct, should be almost always avoided.

I agree, of course. Maybe I wasn't clear in my description, I don't advocate it as a "design pattern". What I meant is that it helps you find bugs, while not being something you should rely on.

It also hides bugs.

Deleting a null pointer is legal and guaranteed by the language to be a no-op. So you if you're double-deleting a pointer in some code path, you have hidden that bug and you won't discover it. At least pick a non-null pointer if you want to clear a pointer to some easy-to-spot value so that it at least is likely to crash or do something to get your attention.




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS