Program crashes when assigning a string.

[Ishida]

I'm sorry if this is in the wrong forum but I have a problem with my program.
For some reason it crashes when I'm trying to assign a string in a constructor.
I'm pretty new to c++ so I'm not sure what I'm doing wrong.

Well anyway here is the code.

Card.cpp

#include "Card.h"

using namespace std;

Card::Card(){
card_name="";
level = 0;
attribute = 0;
attack=0;
defense=0;
picture = NULL;
}


Card::Card(string n_name,unsigned short newlevel,unsigned short newattribute,unsigned short newatk,unsigned short newdef,char *filename){
level = newlevel;
//card_name = "";  //this works.
//card_name=n_name; //crashes
card_name="random string"; //also crashes

attribute = newattribute;
attack=newatk;
defense=newdef;
picture = load_bitmap( filename , NULL); // Load our picture                     
}

Card::~Card(){
}

Card.h

#ifndef CARD_H
#define CARD_H

#include "allegro.h"
#include <iostream>
#include <string>

using std::ostream;
using std::string;



class Card {

public:
	Card();
	Card(string cname,unsigned short lvl, unsigned short atr,unsigned short atk,unsigned short def,char *f);
~Card ();
	unsigned short level;
	unsigned short attribute;
	unsigned short attack;
	unsigned short defense;
    BITMAP *picture; 
    string card_name;
};


#endif

Deck2.h

#ifndef DECK2_H
#define DECK2_H

#include "Card.h"
#include <vector>
#include <algorithm>
#include <ctime>
#include <functional>

using namespace std;

// random generator function:
ptrdiff_t myrandom (ptrdiff_t i) { return rand()%i;}

// pointer object to it:
ptrdiff_t (*p_myrandom)(ptrdiff_t) = myrandom;

class Deck2 {

public:

Card* pakk;	

	Deck2 () {
	pakk=NULL;
	kaarte=0;
	}
	
	Deck2 (unsigned short kaartideArv) {
	pakk=NULL;
	kaarte=0;
	generateDeck(kaartideArv);
	sortDeck();
	}
	

~Deck2 (){
delete [] pakk;
}

void sortDeck(){
srand(time(0));
vector<Card> kaartVektor (pakk, pakk+kaarte);
random_shuffle(kaartVektor.begin(),kaartVektor.end(),p_myrandom);
copy(kaartVektor.begin(), kaartVektor.end(), pakk) ;
}

unsigned int getKaarte(){
return kaarte;         
}

Card takeCard(){
Card tempcard;
tempcard=pakk[kaarte-1];
vector<Card> kaartVektor (pakk, pakk+kaarte);
kaartVektor.pop_back();
kaarte--;
copy(kaartVektor.begin(), kaartVektor.end(), pakk) ;
return tempcard;
}

void addCard (Card card){

Card* temparray;
temparray = new Card[kaarte+1];
memcpy(temparray, pakk, sizeof(Card) * kaarte);

delete [] pakk;
pakk = new Card[kaarte+1];

memcpy(pakk, temparray, sizeof(Card) * kaarte);
pakk[kaarte]=card;

delete [] temparray;

kaarte++;

}

void generateDeck(unsigned short kaartideArv){

Card kaardipakk[]={
     Card("Dark Elf",0,0,0,0,"cards/monsters/dark_elf.bmp")
     };

kaarte=kaartideArv;
pakk = new Card[kaarte];
memcpy(pakk, kaardipakk, sizeof(Card) * kaarte);
sortDeck();
}

Card& operator[] (unsigned int i){
return pakk[i];
}

private:

unsigned int kaarte;


};

#endif

 Kaardimang.cpp
#include "Card.h"
#include "Deck2.h"
#include <cstdlib>
#include <math.h>

using namespace std;

int main (int argc, char* argv[]) {
	// Initialize the Allegro library	
	if (allegro_init () != 0) {
		allegro_message ("Error initializing the Allegro library!");
		return EXIT_FAILURE;
	}

	// Set up Allegro functions
	install_keyboard ();
	install_timer ();
 	   
	install_mouse ();

	// Set a windowed graphics mode, 800x600 windowed, 32-bit color depth
	set_color_depth (desktop_color_depth ());
	if (set_gfx_mode (GFX_AUTODETECT_WINDOWED, 800, 600, 0, 0) != 0) {
		allegro_message ("Error creating 800x600x32 window!");
		return EXIT_FAILURE;
	}


Deck2 pakk(1);

BITMAP *buffer = NULL; // Declare a BITMAP called buffer.
buffer = create_bitmap(800,600); //Create an empty bitmap.

show_mouse(screen);

while (!key[KEY_ESC]){
blit(buffer, screen, 0,0,0,0,800,600);//Draw the buffer to the screen
clear_bitmap(buffer); // Clear the contents of the buffer bitmap 
}
destroy_bitmap(buffer); // Release the bitmap data 


	return EXIT_SUCCESS;
}


// NB! This is required for Allegro to maintain platform compatibility
END_OF_MAIN()

[laserlight]

One possibility is that you messed up elsewhere, but the effects of a mistake that say, writes into memory that should not be written to at that point, surfaces when you try to assign a non-empty string to the card_name member variable.

By the way, passing std::string objects by (const) reference unless you really want to make a copy. The filename parameter probably should be a const char*. Oh, and use the constructor initialisation list instead of assigning in the constructor body.

You should not use using declarations (e.g., using std::string) and using directives (e.g., using namespace std) in a header file except within some restricted scope.

[Paul McKenzie]

I'm sorry if this is in the wrong forum but I have a problem with my program.
For some reason it crashes when I'm trying to assign a string in a constructor.

Why are you doing this?

void addCard (Card card){

Card* temparray;
temparray = new Card[kaarte+1];
memcpy(temparray, pakk, sizeof(Card) * kaarte);

delete [] pakk;
pakk = new Card[kaarte+1];

memcpy(pakk, temparray, sizeof(Card) * kaarte);
pakk[kaarte]=card;

delete [] temparray;

kaarte++;

And at the same time, you're including <vector>? Why not just use vector instead of all that code above? You are doing exactly what vector is supposed to do. You repeat the same kind of coding in other places. Replace any code that looks like this and just use vector. It makes no sense to be coding your own dynamic arrays, and at the same time, your code is using a class that does dynamic arrays, i.e. vector.

Regards,

Paul McKenzie

[laserlight]

Why are you doing this?

Good catch, especially since Card is not a POD type so the memcpy() is suspect.

By the way, Ishida, please indent your code properly.

[Paul McKenzie]

Good catch, especially since Card is not a POD type so the memcpy() is suspect.

Right. The memcpy() invokes undefined behaviour, since Card is non-POD.

Ishida, you cannot use memcpy() on an array of Card objects. The Card object contains a std::string, therefore the only way to copy is to use the proper copy construction and assignment.

That's why "pakk" should either be a vector<Card> or vector<Card*>, more than likely a vector<Card*> due to the BITMAP* member in the Card class.

Regards,

Paul McKenzie

[Ishida]

Removed the memcpy() and used the vector instead and now it works perfectly!

Thanks for the help, really appreciated it. =)

[Lindley]

The only subtlety to be aware of is that if you don't define your open copy constructor and operator=, it will be possible for multiple Cards to share a pointer to the same BITMAP.

If that's not a problem, then don't worry about it.