Deep copy problem

[JohnCastanza]

Im trying to make a game. Theres a player and he shoots bullets, and i was told whats wrong is it is making a shallow copy instead of a deep copy even though i researched this i cannot completely understand it. This is my attempt however i know it is wrong in some way because whenever i call the fire() method, the program crashes. I am VERY desperate i have been trying to solve this for a very long time and would appreciate any help!

Bullet Class:

#include "Bullet.h"

Bullet::Bullet()
{
	ptr = NULL;
}

Bullet::~Bullet() {
  delete ptr;
  ptr = NULL;
}

Bullet& Bullet::operator= (Bullet const& f) {
  if ( this != &f ) {
    delete ptr;
    ptr=new char[strlen(f.ptr)+1];
    strcpy(ptr,f.ptr);
  }
  return *this;
}

Bullet::Bullet(const char *p) {
  ptr=new char[strlen(p)+1];
  strcpy(ptr,p);
}

Bullet::Bullet ( Bullet const& f) {
  ptr=new char[strlen(f.ptr)+1];
  strcpy(ptr,f.ptr);
}

Bullet::Bullet(int startX, int startY, int moveX, int moveY)
{
	startingX = startX;
	startingY = startY;
	x = 0;
	y = 0;
	dx = moveX;
	dy = moveY;
	Extra ex;
}

int Bullet::getX()
{
	return x;
}

int Bullet::getY()
{
	return y;
}

int Bullet::getDx()
{
	return dx;
}

int Bullet::getDy()
{
	return dy;
}

int Bullet::getStartingX()
{
	return startingX;
}

int Bullet::getStartingY()
{
	return startingY;
}

void Bullet::move()
{
	x += dx;
	y += dy;
}

void Bullet::show(SDL_Surface *bullet, SDL_Surface *screen, SDL_Rect camera)
{
	
	//Show the bullet
    ex.apply_surface( (startingX  - camera.x) + x, (startingY - camera.y) + y, bullet, screen );
}

fire() method:

#include "Dot.h"

using namespace std;

Dot::Dot()
{
    //Initialize the offsets
	mousex = 0;
	mousey = 0;
	angle = 0;
	imageOffsetX = 15;
	imageOffsetY = 15;
	dx = 1;
	dy = 1;
	bullets = 20;
    velocity = 3;
	box.x = 32;
    box.y = 32;
    box.w = 20;
    box.h = 20;
    TYPE_WOOD = 0;
    TYPE_ROCK = 1;
    TOTAL_TILES = 2304;

	//Bullets
	vector<Bullet> playerBullets;

    //Path to extra function
    Extra ex;
	
    //Initialize the velocity
    xVel = 0;
    yVel = 0;
}

void Dot::fire()
{

	if( bullets > 0 )
	{

		if (angle == 0){
			playerBullets.push_back( Bullet(box.x, box.y, 5, 0) );
		}else if(angle == 90){
			playerBullets.push_back( Bullet(box.x, box.y, 0, -5) );
		}else if(angle == 180){
			playerBullets.push_back( Bullet(box.x, box.y, -5, 0) );
		}else if(angle == 270){
			playerBullets.push_back( Bullet(box.x, box.y, 0, 5) );
		}
	}
}

[Disch]

I don't see anything there that would cause a crash. Where does it crash?

I also don't see any shallow copies, unless you have a pointer other than 'ptr' in your Bullet class.


You are leaking memory though. If you use new[], as you are, you must use delete[], not delete.

That is:

delete[] ptr;  // <- not delete ptr 

Although -- you could just use strings instead of char arrays, then you don't have to deal with all that crap.

[JohnCastanza]

I made the change from delete to delete[], thanks :)

The program crashes when i execute either of these in the fire() method above:
- playerBullets.push_back( Bullet(box.x, box.y, 5, 0) );
- playerBullets.push_back( Bullet(box.x, box.y, 0, -5) );
- playerBullets.push_back( Bullet(box.x, box.y, -5, 0) );
- playerBullets.push_back( Bullet(box.x, box.y, 0, 5) );

I'm guessing i didnt incorporate all the necessary functions for deep copying correctly and it spawned some sort of memory leak and crashes.

If you need to look at my main i can post it if necessary

[webJose]

The constructor you use, namely Bullet(int, int, int, int), doesn't initialize ptr. Therefore, when the time comes to copy (in the push_back() process), the copy constructor attempts to copy a bogus string, and the app crashes.

Correct the delete as Disch said.

Now I say: Move to initializing lists:

Bullet::Bullet(int startX, int startY, int moveX, int moveY)
    : startingX(startX), startingY(startY), x(0), y(0), dx(moveX), dy(moveY)
        , ptr(NULL) // Super important!!  All constructors must initialize all variables.
{
    //Now what is this for???????  It declares a variable ex of type Extra and does nothing else.
    Extra ex;
}

As you see in the note in code, you must always initialize all variables from all constructors. You are not doing this. Not doing this brought you here in the first place.

[JohnCastanza]

Gotcha,

The ex class variable is just a path needed for my SDL function. Not relevent though so i didnt touch on it.

I took your suggestion and initialized my ptr variable in both my default and 4 param constructor/destructor like so:

#include "Bullet.h"

Bullet::Bullet()
{
	ptr = NULL;
}

Bullet::~Bullet() {
  delete[] ptr;
  ptr = NULL;
}

Bullet& Bullet::operator= (Bullet const& f) {
  if ( this != &f ) {
    delete ptr;
    ptr=new char[strlen(f.ptr)+1];
    strcpy(ptr,f.ptr);
  }
  return *this;
}

Bullet::Bullet(const char *p) {
  ptr=new char[strlen(p)+1];
  strcpy(ptr,p);
}

Bullet::Bullet ( Bullet const& f) {
  ptr=new char[strlen(f.ptr)+1];
  strcpy(ptr,f.ptr);
}

Bullet::Bullet(int startX, int startY, int moveX, int moveY)
{
	startingX = startX;
	startingY = startY;
	x = 0;
	y = 0;
	dx = moveX;
	dy = moveY;
	ptr = NULL;
	Extra ex;
}

int Bullet::getX()
{
	return x;
}

int Bullet::getY()
{
	return y;
}

int Bullet::getDx()
{
	return dx;
}

int Bullet::getDy()
{
	return dy;
}

int Bullet::getStartingX()
{
	return startingX;
}

int Bullet::getStartingY()
{
	return startingY;
}

void Bullet::move()
{
	x += dx;
	y += dy;
}

void Bullet::show(SDL_Surface *bullet, SDL_Surface *screen, SDL_Rect camera)
{
	//Show the bullet
    ex.apply_surface( (startingX  - camera.x) + x, (startingY - camera.y) + y, bullet, screen );
}

Heres the header:

#include "Extra.h"

class Bullet{

private:
	int x, y;
	int dx, dy;
	char *ptr;

	Extra ex;

public:
	Bullet();
	~Bullet();
	Bullet& operator= (Bullet const& f);
	Bullet ( Bullet const& f);
	Bullet(const char *p);
	
	Bullet(int,int,int,int);
	
	int startingX, startingY;
	int getX();
	int getY();
	int getDx();
	int getDy();
	int getStartingX();
	int getStartingY();

	void move();
	void show(SDL_Surface*, SDL_Surface*, SDL_Rect);

};
#endif 

That seemed like the reasonable solution but this is still crashing my program unfortunetly

[Disch]

If you are assigning your pointer to NULL, you'll need to check for null pointers as a special case whenever you read the string.

Example:

Bullet& Bullet::operator= (Bullet const& f) {
  if ( this != &f ) {
    delete ptr;
    ptr=new char[strlen(f.ptr)+1];  // this will crash if f.ptr is NULL
    strcpy(ptr,f.ptr);  // so will this
  }
  return *this;
}

Really -- you're making this much harder on yourself than it needs to be. Just use a string. Don't manage memory unless you have to -- and when you do have to, put it in its own class.

[webJose]

Disch is correct. Also note that operator= continues to use the incorrect delete (must be delete[]).

Remember: You can delete a pointer pointing to NULL without issue, but that's about it. For everything else you need to check on the pointer and avoid operating on it if it points to NULL. Disch shows a good example: The use of strlen() on NULL crashes the application.

Also note that you disregarded my advice for initialization lists and you also disregarded my advice of always initializing all variables in all constructors. Although this might not be required to get rid of your particular problem, it may very well cause you grief in the future. Remember that your way of initializing variables inside the constructor's code is actually using their operator= implementation and not their constructors.