Segmentation fault with std::list

[Delca]

Hi all,

I have this class named "room" :

class room{

  private :

    int* array;
    int xlength,ylength;

  public :

    room(int* data, int xL, int yL);
    int  GetXLength();
    int  GetYLength();
    int* GetArray();
    void ChangeRoom(int* data, int xL, int yL);
    void InsertIntoArray(int* array2, int dim2x, int x, int y);
    room* operator=(room* copy);
    int operator<(room* other);
    int operator==(room* other);
    ~room();

           };

The problem is, when I use the list<room> type, the program will sometimes get a Segmentation fault. The exact error I get from the debugger is

#0 0044D2D6	std::_List_base<room, std::allocator<room> >::_M_clear(this=0x28fe80) (u:/program files (x86)/codeblocks/mingw/bin/../lib/gcc/mingw32/4.4.1/include/c++/bits/list.tcc:72)
#1 0044D39F	~_List_base(this=0x28fe80) (u:/program files (x86)/codeblocks/mingw/bin/../lib/gcc/mingw32/4.4.1/include/c++/bits/stl_list.h:360)
#2 004656C9	~list(this=0x28fe80) (u:/program files (x86)/codeblocks/mingw/bin/../lib/gcc/mingw32/4.4.1/include/c++/bits/stl_list.h:418)
#3 00401A5D	main() (U:\CodeBlocksProjects\DungeonGenerator\main.cpp:130)

After some research, I thought that the problem was the "int* array", which is dynamically allocated in room::room, and then destroyed with "delete [] array;" in room::room()~.

room::room(int* data, int xL, int yL){

  xlength=xL;
  ylength=yL;

  int i,j;

  array = new int[xL*yL];

  for(i=0;i<xL;i++){
  for(j=0;j<yL;j++){

     array[j*xL + i] = data[j*xL + i];

  }
  }



}

room::~room(){

  delete [] array;
  array=NULL;

}

But I can't find the flaw in my code. Does someone see where the problem is ?

[Catfish]

Does data store at least xL*yL elements?

You're using pointers in so many places, I can't tell where the problem might come from.
And the debugger output mentions std::list but I don't see it used in your code.

Edit: never mind that last part.

[ne555]

I'll bet to the copy constructor and assignment operator.
It is weird that you request for a pointer in your methods.

[zb0S216C]

Just a heads up: In room::room(), if int *data is a dynamically allocated array, why not make int* data a reference to a pointer? It's not essential, but the method makes more sense with it.

_Wazzak

[DSLq5Di1]

ne555 wrote:

I'll bet to the copy constructor

^ Which seems to be missing unless my eyes deceive me..

[ne555]

That is the point. The compiler provide a trivial copy constructor, that it will copy every member.
So you end with a shallow copy, that causes a double delete.

@Framework: ¿? it is not modifying 'data' ¿why passing it by reference?

[Delca]

I tried to add the copy constructor, but nothing changed. Maybe I did it wrong ?

room::room(room* copy){

  xlength=copy->GetXLength();
  ylength=copy->GetYLength();

  int i,j;

  delete [] array;

  array = new int[xlength*ylength];

  for(i=0;i<xlength;i++){
  for(j=0;j<ylength;j++){

    array[j*xlength + i] = copy->GetArray()[j*xlength + i];

  }
  }

}

Also, I changed my methods by replacing "int* array" with "int array[]". But passing an array means giving a pointer to its first element, right ? So does it make a difference ?

[DSLq5Di1]

Not quite right, a copy constructor takes a reference. Remember that this object will be constructed from the object passed in, there has been no initialization yet, and as such delete [] array; would likely cause a run time error.

You simply need to copy the members of the other object,

room(const room& copy)
	: array(new int[copy.xlength * copy.ylength]),
	  xlength(copy.xlength), ylength(copy.ylength)
{
	for (size_t i = 0; i < xlength * ylength; ++i)
		array[i] = copy.array[i];
}

[Delca]

Ah, right. I already have a function which allows to change all the private variables (which is ChangeRoom), and I forgot to erase it after copying the code. I also changed my function so that the argument's type matches yours.

Still, I get the same error.

EDIT : I think I found what was causing the problem. I was creating a room inside a function as a local variable, then adding this room to a list<room> which was given to the function as a pointer.
Rewriting the function so that it returns the room and then adding this room to the list and deleting it after caused no problem.
But I don't understand why the problem was solved. Adding an element to a list creates a copy of this element, doesn't it ?

[ne555]

Sorry I don't understand your description, ¿could you post code?

then adding this room to the list and deleting it after

¿deleting? You can't delete objects.

[Delca]

Err... I don't have the wrong version anymore, but it was something like that :

void function(std::list<room> roomlist, ...){

room* newroom = new room(...);

roomlist.push_front(&newroom);

delete newroom;

}

While the new function returns a "room" :

room createroomfromfile(std::string filename){

std::ifstream file;

file.open( filename.c_str() );

if (file.good())
{
    int xlength, ylength, temp;
    file>>xlength;
    file>>ylength;

    int data[xlength*ylength];
    int i;
    for(i=0;i<xlength*ylength;i++){

    if (file >> temp) {data[i]=temp;}

    }

file.close();

room newroom(data, xlength, ylength);

return newroom;

}

}

which is then added to the list in main(); .

(I know that I did bad things when reading the int from the text file ; but as I only give correct text files, I planned to rework it later...)

[zb0S216C]

ne555 wrote:

¿? it is not modifying 'data' ¿why passing it by reference? (sic)

"I want to work with that pointer." I stand strong by passing the pointer by reference; end-of.

<sub>I hate implicit type-conversions. Make sure a type-conversion is explicit. You don't want a buggy program, now, do you?

Wazzak</sub>