Assignment operator overloading trouble

[Sy0XoG1T]

Alright so I just need someone to tell me if I'm coding this assignment operator correctly (=). It's been giving me trouble all night...

ec::Sprite & ec::Sprite::operator = (const Sprite & Copy)
{
	delete Base;
	delete Image;
	delete Filepath;
	delete X, Y, TOP, BOTTOM, LEFT, RIGHT, WIDTH, HEIGHT;

	this->Image = new sf::Image;
	this->Image->LoadFromFile(*Copy.Filepath);
	this->Base = new sf::Sprite(*Image);
	this->Base->SetCenter((float)this->Image->GetWidth() / 2, (float)this->Image->GetHeight() / 2);
	this->Base->SetPosition(*Copy.X, *Copy.Y);

	this->X		= new float(*Copy.X);
	this->Y		= new float(*Copy.Y);
	this->HEIGHT	= new float((float)this->Image->GetHeight());
	this->WIDTH	= new float((float)this->Image->GetWidth());
	this->TOP	= new float(this->Base->GetPosition().y - (*this->HEIGHT / 2));
	this->BOTTOM	= new float(this->Base->GetPosition().y + (*this->HEIGHT / 2));
	this->LEFT	= new float(this->Base->GetPosition().x - (*this->WIDTH / 2));
	this->RIGHT	= new float(this->Base->GetPosition().x + (*this->WIDTH / 2));

	return *this;
}

Here's the class if you need to take a look at it as well...

namespace ec
{
	////////////////////////////////////////////////////////////
	/// Base abstract class for drawable classes.
	///
	////////////////////////////////////////////////////////////
	class Drawable
	{
	protected:
		////////////////////////////////////////////////////////////
		/// SFML base abstract class.
		///
		////////////////////////////////////////////////////////////
		sf::Drawable * Base;

		////////////////////////////////////////////////////////////
		/// Dimensions.
		///
		////////////////////////////////////////////////////////////
		float *X, *Y;
		float *TOP, *BOTTOM, *LEFT, *RIGHT;
		float *WIDTH, *HEIGHT;

	public:
		////////////////////////////////////////////////////////////
		/// Default constructor and destructor.
		///
		////////////////////////////////////////////////////////////
		Drawable();
		~Drawable();

		////////////////////////////////////////////////////////////
		/// Virtual offset functions.
		///
		////////////////////////////////////////////////////////////
		virtual void OFFSET(const float, const float) = 0;
		virtual void OFFSET(const sf::Vector2f) = 0;

		////////////////////////////////////////////////////////////
		/// Virtual set position functions.
		///
		////////////////////////////////////////////////////////////
		virtual void SETPOS(const float, const float) = 0;
		virtual void SETPOS(const sf::Vector2f) = 0;

		////////////////////////////////////////////////////////////
		/// Operators.
		///
		////////////////////////////////////////////////////////////
		//Drawable operator = (const Drawable);

		////////////////////////////////////////////////////////////
		/// Allows Core to use private functions.
		///
		////////////////////////////////////////////////////////////
		friend class Core;
	};

	////////////////////////////////////////////////////////////
	/// Derived class from Drawable used for sprites.
	///
	////////////////////////////////////////////////////////////
	class Sprite : public Drawable
	{
	private:
		////////////////////////////////////////////////////////////
		/// Holds the image of the sprite.
		///
		////////////////////////////////////////////////////////////
		sf::Image * Image;

		////////////////////////////////////////////////////////////
		/// Filepath.
		///
		////////////////////////////////////////////////////////////
		const std::string * const Filepath;

		////////////////////////////////////////////////////////////
		/// Updates the sprite if the coordinates change.
		/// Does not get called outside of Sprite.
		///
		////////////////////////////////////////////////////////////
		void UPDATE();

	public:
		////////////////////////////////////////////////////////////
		/// Constructor and destructor.
		///
		////////////////////////////////////////////////////////////
		Sprite(const std::string Filepath, const float X = 0, const float Y = 0);
		~Sprite();

		////////////////////////////////////////////////////////////
		/// Operators.
		///
		////////////////////////////////////////////////////////////
		Sprite & operator = (const Sprite & Copy);

		////////////////////////////////////////////////////////////
		/// Offset the sprite by the given amounts.
		///
		/// Example:
		/// OFFSET(4, -2) will move the image right by 4 and up by 2
		///
		////////////////////////////////////////////////////////////
		void OFFSET(const float OffsetX, const float OffsetY);
		void OFFSET(const sf::Vector2f Vector2f);

		////////////////////////////////////////////////////////////
		/// Set the sprite in a new position.
		///
		////////////////////////////////////////////////////////////
		void SETPOS(const float X, const float Y);
		void SETPOS(const sf::Vector2f Vector2f);

		////////////////////////////////////////////////////////////
		/// Rotation.
		///
		////////////////////////////////////////////////////////////
		void SETANGLE(const double Degrees);
		void SETANGLE(const float Degrees);
		void ROTATE(const double Degrees);
		void ROTATE(const float Degrees);

	};
}

Any thoughts?

[Athar]

Any thoughts?

Yes. Why are X, Y, Filepath etc. dynamically allocated? With that much unnecessary manual memory management you set the cornerstone for a myriad of problems.

Also, why are WIDTH and HEIGHT floats? Are you measuring size in something different than pixel units?

[Sy0XoG1T]

Didn't think having all of those as pointers would really effect anything....and no WIDTH and HEIGHT are actually supposed to be unsigned int, I messed up.

Are those really the cause of my runtime error though? I really think it's because I screwed up somewhere in overloading the operator.

[melkiy]

I see two potentially dangerous things in your code.

1) Are you sure that you delete correctly allocated memory at the beginning of the operator= ?
Do assign them NULL or valid values in the constructor!

2) In the case of the Copy == *this you'd got a problem. operator= must look like this:

MyClass& operator= (const MyClass& mc)
{
  if( this != &mc )
  {
     //do all the stuff only if (*this) and mc are not the same object
  }
  return *this;
}

Allso i agree that when you rewrite it all without pointers the problem (1) will most probably disappear.

[Galik]

It really doesn't make any sense storing things like float using pointers unless you absolutely have to. The pointer may take up more memory than the float itself. It will also be slower to access and it is error prone.

[Galik]

This:
delete X, Y, TOP, BOTTOM, LEFT, RIGHT, WIDTH, HEIGHT;

Will only actually delete HEIGHT. nothing else.

[Athar]

inlinevoid wrote:

Are those really the cause of my runtime error though?

Yes, most likely.

inlinevoid wrote:

I really think it's because I screwed up somewhere in overloading the operator.

Yes, most likely. The most probable reason you screwed up is throwing around all these pointers in the first place.

Without any need, I'd like to repeat.
Using pointers in this case is
a) slow - additional allocations and deallocation are necessary
b) even slower - additional dereferencing is necessary
c) still slower - locality of reference can be affected
d) wastes memory for the pointers
e) wastes memory for additional memory management overhead
f) inflates your code - all these deletes and new floats would be unnecessary
g) and the biggest problem of all that I already mentioned: a bottomless source for errors. Especially memory leaks, since you won't notice right away that they even exist. Because unlike forgotten allocations, they don't tend to blow up in your face.

[Sy0XoG1T]

Took away all of the pointers but I still get the same result....

Sprite functions...

////////////////////////////////////////////////////////////
/// Sprite constructor.
///
////////////////////////////////////////////////////////////
ec::Sprite::Sprite(const std::string Filepath, const float X, const float Y):
Filepath(Filepath)
{
	Image.LoadFromFile(Filepath);
	Base = new sf::Sprite(Image);
	Base->SetCenter((float)Image.GetWidth() / 2, (float)Image.GetHeight() / 2);
	Base->SetPosition(X, Y);


	this->X		= X;
	this->Y		= Y;
	HEIGHT		= Image.GetHeight();
	WIDTH		= Image.GetWidth();
	TOP			= Base->GetPosition().y - (HEIGHT / 2);
	BOTTOM		= Base->GetPosition().y + (HEIGHT / 2);
	LEFT		= Base->GetPosition().x - (WIDTH / 2);
	RIGHT		= Base->GetPosition().x + (WIDTH / 2);
}

////////////////////////////////////////////////////////////
/// Sprite destructor.
///
////////////////////////////////////////////////////////////
ec::Sprite::~Sprite()
{
}

////////////////////////////////////////////////////////////
/// Operators.
///
////////////////////////////////////////////////////////////
ec::Sprite & ec::Sprite::operator = (const Sprite & Copy)
{
	if(this != &Copy)
	{
		delete Base;

		Image = Copy.Image;
		Image.LoadFromFile("sprites/test.tga");
		Base = new sf::Sprite(Image);
		Base->SetCenter((float)this->Image.GetWidth() / 2, (float)this->Image.GetHeight() / 2);
		Base->SetPosition(Copy.X, Copy.Y);

		X			= Copy.X;
		Y			= Copy.Y;
		HEIGHT		= Image.GetHeight();
		WIDTH		= Image.GetWidth();
		TOP			= Base->GetPosition().y - (HEIGHT / 2);
		BOTTOM		= Base->GetPosition().y + (HEIGHT / 2);
		LEFT		= Base->GetPosition().x - (WIDTH / 2);
		RIGHT		= Base->GetPosition().x + (WIDTH / 2);
	}

	return *this;
}

Drawable's functions (if it's relevant)

////////////////////////////////////////////////////////////
/// Default constuctor.
///
////////////////////////////////////////////////////////////
ec::Drawable::Drawable(){}

////////////////////////////////////////////////////////////
/// Destructor.
///
////////////////////////////////////////////////////////////
ec::Drawable::~Drawable()
{
	delete Base;
}

Here's the class again without all of the pointers (removed all the comments so it's not taking up a full page here, I'm sure you can figure it out)....

namespace ec
{
	class Drawable
	{
	protected:

		sf::Drawable * Base;

		float X, Y;
		float TOP, BOTTOM, LEFT, RIGHT;
		unsigned int WIDTH, HEIGHT;

	public:

		Drawable();
		~Drawable();

		friend class Core;
	};

	class Sprite : public Drawable
	{
	private:

		sf::Image Image;

		const std::string Filepath;

		void UPDATE();

	public:
		Sprite(const std::string Filepath, const float X = 0, const float Y = 0);
		~Sprite();

		Sprite & operator = (const Sprite & Copy);

	};
}

Maybe it's something I'm doing wrong in main?

ec::Core * Core;

int main()
{
	Core = new ec::Core(640, 480, 32, 60, "Game", sf::Style::Close);

	ec::Sprite NewSprite("sprites/test.tga", 400, 200);
	ec::Sprite NewNewSprite = NewSprite;

	while(Core->Update())
	{
		Core->Draw(NewSprite);
		Core->Draw(NewNewSprite);
	}

	delete Core;
	return 0;
}

To be more precise about the problem the image from NewSprite draws just fine but when I assign NewNewSprite to NewSprite and try to draw it nothing appears and when I close out of the program I get an unhandled exception.

(If you're thinking that the reason I cant see the other image is because they would be ontop of each other that's not the case, I omited some code that separated the two sprites)

[Athar]

ec::Sprite NewNewSprite = NewSprite;
looks like it might invoke operator=, but it actually invokes the copy constructor, which you haven't defined.
The automatically generated one simply copies the Base pointer, which is undesirable and leads to a double free in the end.
That's why you should follow the Law of the Big Three:
http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

[Sy0XoG1T]

Oooooooh alright.

So

ec::Sprite NewNewSprite;
NewNewSprite = NewSprite;

is what invokes operator=.....I get it now. Thanks