Stack around variable being corrupted

Started by
4 comments, last by Zahlman 15 years, 8 months ago
I've looked at a couple other threads about this here, but nothing seemed to apply to this case. The "stack around variable 'tank1' has been corrupted" error only pops up when I call on tank1's Fire() function, if it is commented out then the program runs and terminates just fine. Program.cpp:

#include "Tank.h"
#include <iostream>

//define Tank constructors
Tank::Tank()
{
	itsX = 0;
	itsY = 0;
	itsZ = 0;
	itsAngle = 0;
	itsHealth = 100;
	itsTarget = NULL;
}

Tank::Tank(float x, float y, float z, float angle)
{
	itsX = x;
	itsY = y;
	itsZ = z;
	itsAngle = angle;
	itsHealth = 100;
	itsTarget = NULL;
}

Tank::~Tank()
{
}

//define Tank functions
void Tank::Fire()
{
	if (itsTarget != NULL)
	{
		//if tank has a target, deal damage
		int thp = itsTarget->GetHealth();
		thp -= 20;
		itsTarget->SetHealth(thp);
	}
	else
	{/**or else do nothing, for now**/}
}

int main()
{
	//make 2 tanks
	Tank tank1;
	Tank tank2;

	//target eachother
	tank1.SetCurrentTarget(tank2);
	tank2.SetCurrentTarget(tank1);

	//std::cout << "Tank1 health: " << tank1.GetHealth() << " Tank2 health: " << tank2.GetHealth() << "\n";
	tank1.Fire();
	//std::cout << "Tank1 fires on Tank2! Tank2 health: " << tank2.GetHealth() << "\n";

	return 0;
}
Tank.h:

#include <iostream>

class Tank
{
private:

	//status data
	
	float itsX;
	float itsY;
	float itsZ;
	float itsAngle;
	int itsHealth;
	Tank * itsTarget;
public:

	//constructors
	Tank();
	Tank(float x, float y, float z, float angle);

	//destructor
	~Tank();

	//action methods
	void Fire();	//tank fires, do damage to itsTarget
	
	//get-set methods for all member variables
	float GetX() const {return itsX;}
	float GetY() const {return itsY;}
	float GetZ() const {return itsZ;}
	float GetAngle() const {return itsAngle;}
	int GetHealth() const {return itsHealth;}
	Tank* GetCurrentTarget() const {return itsTarget;}
	void SetX(float x) {itsX = x;}
	void SetY(float y) {itsY = y;}
	void SetZ(float z) {itsZ = z;}
	void SetAngle(float angle) {itsAngle = angle;}
	void SetHealth(int health) {itsHealth = health;}
	void SetCurrentTarget(Tank target) {itsTarget = &target}

};
I'm still a bit of a noob with C++. Most of my experience is with Java/C#, so I'm sure its something stupid that I'm just missing.
Advertisement
You probably want SetCurrentTarget() to look something like:

void SetCurrentTarget(Tank * target) {itsTarget = target;}
SetCurrentTarget passes by copy, therefore you're storing a pointer to a temporary stack object, which soon after causes a stack corruption

-ja

(Edit:beaten. damn you SiCrane...)
That did the trick, runs just fine now. Thanks!
Could a compiler have warned about storing the address of a temporary variable? It does warn when returning such an address but...
I don't know of any compilers that do, but it's an interesting suggestion.

Anyway. To the OP: this isn't how OO programming works. The point of this whole "encapsulation" thing isn't just making functions to modify each member variable. That just makes more work for you and doesn't really *hide* anything. The point is that outside code that uses a Tank shouldn't *care*, to the extent that that's possible, what data is in it. It also means that the Tank takes responsibility for figuring out what happens to itself, in response to the outside world.

Thus, instead of reading a Tank's hp, decreasing the value and setting it back, you tell the tank "hey, I'm doing X amount of damage to you". Then the Tank adjusts its hp accordingly (which also gives it a chance to apply any special armouring that it has, etc.)

	//int GetHealth() const {return itsHealth;}	//void SetHealth(int health) {itsHealth = health;}	void TakeDamage(int damage) {		// For now...		itsHealth = std::max(0, itsHealth - damage);		if (!itsHealth) { die(); }	}


That kind of thing.

If you're coming from a Java/C# background, you're supposed to know this kind of stuff. Unfortunately, it doesn't really get taught very well.

This topic is closed to new replies.

Advertisement