C++ - Problema con operador delete

[xaps]

Estoy desarrollando una clase Matriz, en la que le he incluido varias sobrecargas de operadores para poder trabajar con estas matrices de una manera más cómoda. El problema viene dado cuando entra en juego el operador de asignación, cuando usa la operación delete para liberar la memoria de la matriz y crear una copia del parámetro, que me da el siguiente error, además de un mapeado de memoria:

Código [Seleccionar] Expandir

*** glibc detected *** ./test.exe: free(): invalid pointer: 0x0000000000401662 ***

Os adjunto el código de la clase y el código de test.exe:

Matriz.h

Código (cpp) [Seleccionar] Expandir


#include <iostream>
#include <vector>

using namespace std;

class Matriz {

private:
 
 int nfilas;
 int ncolumnas;
 int **matr;

public:

 //Constructoras
 Matriz();
 Matriz(int filas, int columnas);
 
 //Destructora
 ~Matriz();
 
 //Consultoras
 int filas() const;
 int columnas() const;
 int consultar(int fila, int columna) const;
 
 //Modificadora
 void modificar(int fila, int columna, int x);
 
 //Entrada / Salida
 void leer();
 void escribir();
 
 //Operadores
 Matriz operator +(const Matriz &b) const;
 Matriz operator -(const Matriz &b) const;
 Matriz operator *(const Matriz &b) const;
 Matriz operator =(const Matriz &b);
 
 bool operator ==(const Matriz &mat) const;
 bool operator !=(const Matriz &mat) const;
};

Matriz::Matriz() {}

Matriz::Matriz(int filas, int columnas)
{
 this->nfilas = filas;
 this->ncolumnas = columnas;
 
 matr = new int* [filas];
 for (int i = 0; i < filas; ++i)
 {
   matr[i] = new int [columnas];
 }
}

Matriz::~Matriz()
{
 delete matr;
}

int Matriz::filas() const
{
 return nfilas;
}

int Matriz::columnas() const
{
 return ncolumnas;
}

int Matriz::consultar(int fila, int columna) const
{
 return matr[fila][columna];
}

void Matriz::modificar(int fila, int columna, int x)
{
 matr[fila][columna] = x;
}

void Matriz::leer()
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j) cin >> matr[i][j];
}

void Matriz::escribir()
{
 for (int i = 0; i < nfilas; ++i)
 {
   for (int j = 0; j < ncolumnas; ++j) cout << matr[i][j] << " ";
   cout << endl;
 }
}

Matriz Matriz::operator +(const Matriz &b) const
{
 int filas = this->nfilas;
 int columnas = this->ncolumnas;
 
 Matriz res(filas, columnas);
 
 for (int i = 0; i < filas; ++i)
   for (int j = 0; j < columnas; ++j)
     res.matr[i][j] = this->matr[i][j] + b.matr[i][j];
   
 return res;
}

Matriz Matriz::operator -(const Matriz &b) const
{
 int filas = this->nfilas;
 int columnas = this->ncolumnas;
 
 Matriz res(filas, columnas);
 
 for (int i = 0; i < filas; ++i)
   for (int j = 0; j < columnas; ++j)
     res.matr[i][j] = this->matr[i][j] - b.matr[i][j];
   
 return res;
}

Matriz Matriz::operator *(const Matriz &b) const
{
 if (this->ncolumnas == b.nfilas)
 {
   int pos = this->ncolumnas;
   int filas = this->nfilas;
   int columnas = b.ncolumnas;
   
   Matriz res(filas, columnas);
   for (int i = 0; i < filas; ++i)
   {
     for (int j = 0; j < columnas; ++j)
     {
int value = 0;
for (int k = 0; k < pos; ++k) value += this->matr[i][k] * b.matr[k][j];
res.matr[i][j] = value;
     }
   };
   return res;
 }
}

Matriz Matriz::operator =(const Matriz &mat)
{
 if (this != &mat)
 {
   cout << "flag1" << endl;
   delete this->matr;
   cout << "flag2" << endl;
   if (mat.matr)
   {
     this->nfilas = mat.nfilas;
     this->ncolumnas = mat.ncolumnas;
     
     matr = new int* [nfilas];
     for (int i = 0; i < nfilas; ++i)
     {
matr[i] = new int [ncolumnas];
     }
     
     for (int i = 0; i < nfilas; ++i)
     {
for (int j = 0; j < ncolumnas; ++j)
{
 matr[i][j] = mat.matr[i][j];
}
     }
   }
   else this->matr = NULL;
 }
 return *this;
}

bool Matriz::operator ==(const Matriz &mat) const
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j)
     if (this->matr[i][j] != mat.matr[i][j]) return false;
 
 return true;
}

bool Matriz::operator !=(const Matriz &mat) const
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j)
     if (this->matr[i][j] != mat.matr[i][j]) return true;
 
 return false;
}


Código (cpp) [Seleccionar] Expandir


#include "Matriz.h"

int main()
{
 int f1, c1, f2, c2;

 cout << "Introduce el tamaño de la primera matriz:" << endl;
 cin >> f1 >> c1;
 Matriz mat1(f1, c1);
 cout << "Introduce los valores de la matriz:" << endl;
 mat1.leer();
 
 cout << "Introduce el tamaño de la segunda matriz:" << endl;
 cin >> f2 >> c2;
 Matriz mat2(f2, c2);
 cout << "Introduce los valores de la matriz:" << endl;
 mat2.leer();

 
 if (c1 == f2)
 {
   Matriz res;
   res = mat1 * mat2;

   res.escribir();
 }
}


He probado varias cosas, como cambiar delete por delete[], o intentar eliminar los vectores individualmente, pero ninguna ha dado resultado. Algo debo estar haciendo mal, pero no encuentro el error. Si le pudierais echar un ojo me haríais un favor.

Muchas gracias.

[amchacon]

Código (cpp) [Seleccionar] Expandir

Matriz::~Matriz()
{
delete matr;
}
Eso está mal hecho, tienes que borrar al contrario que como la creas (primero con un for borras todos los vectores y despues borras el puntero principal).

Código (cpp) [Seleccionar] Expandir

Matriz::~Matriz()
{
for (int i = 0; i < nfilas;i++) delete[] matr[i];
delete[] matr;
}

[xaps]

Código (cpp) [Seleccionar] Expandir

Matriz::~Matriz()
{
delete matr;
}
Eso está mal hecho, tienes que borrar al contrario que como la creas (primero con un for borras todos los vectores y despues borras el puntero principal).

Código (cpp) [Seleccionar] Expandir

Matriz::~Matriz()
{
for (int i = 0; i < nfilas;i++) delete[] matr[i];
delete[] matr;
}

Gracias por la explicación. Eso es a lo que me refería cuando decía de eliminar los vectores individualmente. No me daba resultado por un error en el código de la sobrecarga del operador de asignación, en el que me pongo a eliminar los datos de matr sin verificar que esté vacío. Pero ahora, una vez arreglado ésto, me ocurre una cosa bastante curiosa que me produce un error de segmentación. Primero adjunto las funciones que he arreglado, y luego comento cual es el error que estoy teniendo ahora:

~Matriz();

Código (cpp) [Seleccionar] Expandir

Matriz::~Matriz()
{
 for (int i = 0; i < this->nfilas; ++i)
 {
   cout << "~fila " << i << endl;
   delete[] this->matr[i];
 }
 delete[] this->matr;
}

Matriz operator =(const Matriz &mat);

Código (cpp) [Seleccionar] Expandir

Matriz Matriz::operator =(const Matriz &mat)
{
 cout << "asignacion" << endl;
 if (this != &mat)
 {
   if (this != NULL)
   {
     cout << "flag1" << endl;
     
     for (int i = 0; i < this->nfilas; ++i)
     {
cout << "fila " << i << endl;
delete[] this->matr[i];
     }
     delete[] this->matr;
   }
   
   cout << "flag2" << endl;
   if (mat.matr)
   {
     this->nfilas = mat.nfilas;
     this->ncolumnas = mat.ncolumnas;
     
     matr = new int* [nfilas];
     for (int i = 0; i < nfilas; ++i)
     {
matr[i] = new int [ncolumnas];
     }
     
     for (int i = 0; i < nfilas; ++i)
     {
for (int j = 0; j < ncolumnas; ++j)
{
 matr[i][j] = mat.matr[i][j];
}
     }
   }
   else this->matr = NULL;
 }
 return *this;
}

Pues bien, el error es el siguiente: Parece que justo antes de realizar la asignación el destructor por defecto se carga la matriz, y cuando en el delete de la sobrecarga de asignación intenta acceder a la posición "i" le da un error de segmentación. He llegado a esta conclusión mediante la salida que me ha dado el programa. Os enseño cual es la entrada que le he dado y cual es la salida que me ha devuelto:

Entrada:

Código [Seleccionar] Expandir


2 2
1 1 0 1
2 2
1 0 1 1


Salida:

Código [Seleccionar] Expandir


~fila 0
~fila 1
asignacion
flag1
fila 0
Violación de segmento


Como podéis comprobar, primero entra al destructor por defecto y a continuación intenta realizar la asignación. El problema es que no se ni que matriz elimina, ni porqué la matriz a la que se le realiza la asignación entra dentro de la condición (teóricamente, la matriz res tiene matr = NULL, ¿no?).

Muchas gracias de nuevo.

[amchacon]

Código (cpp) [Seleccionar] Expandir

if (this != NULL)

Esa condición siempre va a ser cierta xD. Creo que te referias a this.matr, supongo que de ahí viene el error de segmentación (borrar cuando ni siquiera tienes matriz).

Por cierto no me parece adecuado que copypastees el destructor ahí, yo me haría una función privada "borrar". Y esa función la llamo desde el destructor y desde allí, por reciclar código más que nada.

[xaps]

Código (cpp) [Seleccionar] Expandir

if (this != NULL)

Esa condición siempre va a ser cierta xD. Creo que te referias a this.matr, supongo que de ahí viene el error de segmentación (borrar cuando ni siquiera tienes matriz).

Por cierto no me parece adecuado que copypastees el destructor ahí, yo me haría una función privada "borrar". Y esa función la llamo desde el destructor y desde allí, por reciclar código más que nada.

Vaya error mas tonto... jajaj Gracias!

Sobre lo de la función, ni se me había ocurrido. Ya lo he implementado.
Otro "error" que tenia era que en la constructora por defecto no iniciaba matr a NULL, y provocaba que una vez arreglado ese error tan tonto me diera violación de segmento de nuevo ya que aún pasaba la condición.

Pero ahora me vuelve a dar un error de nuevo con la destructora (siento ser tan pesado, pero aún no hemos dado ni memoria dinámica en la universidad y lo estoy haciendo por simple entretenimiento y curiosidad, por lo que estoy investigando por cuenta propia y hay cosas que se me escapan). Primero de todo, volveré a adjuntar la clase tal como la tengo ahora, y a continuación pegaré la salida que me da el programa.

Clase Matriz:

Código (cpp) [Seleccionar] Expandir


#include <iostream>
#include <vector>

using namespace std;

class Matriz {

private:
 
 int nfilas;
 int ncolumnas;
 int **matr;
 
 void borrar();

public:

 //Constructoras
 Matriz();
 Matriz(int filas, int columnas);
 
 //Destructora
 ~Matriz();
 
 //Consultoras
 int filas() const;
 int columnas() const;
 int consultar(int fila, int columna) const;
 
 //Modificadora
 void modificar(int fila, int columna, int x);
 
 //Entrada / Sortida
 void leer();
 void escribir();
 
 //Operadores
 Matriz operator +(const Matriz &b) const;
 Matriz operator -(const Matriz &b) const;
 Matriz operator *(const Matriz &b) const;
 Matriz operator =(const Matriz &b);
 
 bool operator ==(const Matriz &mat) const;
 bool operator !=(const Matriz &mat) const;
};

Matriz::Matriz() {matr = NULL;}

Matriz::Matriz(int filas, int columnas)
{
 this->nfilas = filas;
 this->ncolumnas = columnas;
 
 matr = new int* [filas];
 for (int i = 0; i < filas; ++i)
 {
   matr[i] = new int [columnas];
 }
}

Matriz::~Matriz()
{
 cout << "destructora" << endl;
 borrar();
}

void Matriz::borrar()
{
 if (this->matr != NULL)
 {
   for (int i = 0; i < nfilas; ++i)
   {
     cout << "fila " << i << endl;
     delete[] matr[i];
   }
   delete[] matr;
   matr = NULL;
 }
}

int Matriz::filas() const { return nfilas;}

int Matriz::columnas() const { return ncolumnas;}

int Matriz::consultar(int fila, int columna) const { return matr[fila][columna];}

void Matriz::modificar(int fila, int columna, int x) { matr[fila][columna] = x;}

void Matriz::leer()
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j) cin >> matr[i][j];
}

void Matriz::escribir()
{
 cout << "flag3" << endl;
 for (int i = 0; i < nfilas; ++i)
 {
   for (int j = 0; j < ncolumnas; ++j) cout << matr[i][j] << " ";
   cout << endl;
 }
}

Matriz Matriz::operator +(const Matriz &b) const
{
 int filas = this->nfilas;
 int columnas = this->ncolumnas;
 
 Matriz res(filas, columnas);
 
 for (int i = 0; i < filas; ++i)
   for (int j = 0; j < columnas; ++j)
     res.matr[i][j] = this->matr[i][j] + b.matr[i][j];
   
 return res;
}

Matriz Matriz::operator -(const Matriz &b) const
{
 int filas = this->nfilas;
 int columnas = this->ncolumnas;
 
 Matriz res(filas, columnas);
 
 for (int i = 0; i < filas; ++i)
   for (int j = 0; j < columnas; ++j)
     res.matr[i][j] = this->matr[i][j] - b.matr[i][j];
   
 return res;
}

Matriz Matriz::operator *(const Matriz &b) const
{
 if (this->ncolumnas == b.nfilas)
 {
   int pos = this->ncolumnas;
   int filas = this->nfilas;
   int columnas = b.ncolumnas;
   
   Matriz res(filas, columnas);
   for (int i = 0; i < filas; ++i)
   {
     for (int j = 0; j < columnas; ++j)
     {
int value = 0;
for (int k = 0; k < pos; ++k) value += this->matr[i][k] * b.matr[k][j];
res.matr[i][j] = value;
     }
   };
   return res;
 }
}

Matriz Matriz::operator =(const Matriz &mat)
{
 cout << "asignacion" << endl;
 if (this != &mat)
 {
   cout << "flag1" << endl;
   borrar();
   cout << "flag2" << endl;
   
   if (mat.matr)
   {
     this->nfilas = mat.nfilas;
     this->ncolumnas = mat.ncolumnas;
     
     matr = new int* [nfilas];
     for (int i = 0; i < nfilas; ++i)
matr[i] = new int [ncolumnas];
     
     for (int i = 0; i < nfilas; ++i)
     {
for (int j = 0; j < ncolumnas; ++j)
 matr[i][j] = mat.matr[i][j];
     }
   }
   else this->matr = NULL;
 }
 return *this;
}

bool Matriz::operator ==(const Matriz &mat) const
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j)
     if (this->matr[i][j] != mat.matr[i][j]) return false;
 
 return true;
}

bool Matriz::operator !=(const Matriz &mat) const
{
 for (int i = 0; i < nfilas; ++i)
   for (int j = 0; j < ncolumnas; ++j)
     if (this->matr[i][j] != mat.matr[i][j]) return true;
 
 return false;
}


Los datos de entrada son los mismos de antes, y la salida es la siguiente:

Código [Seleccionar] Expandir


destructora
fila 0
fila 1
asignacion
flag2
destructora
fila 0
fila 1
destructora
fila 0
*** glibc detected *** ./test.exe: double free or corruption (out): 0x0000000001a590e0 ***

Además del mapeado de memoria que se da con este tipo de error.

Se puede ver como la asignación aparentemente la realiza bien, pero luego, antes de ejecutar la función leer() elimina dos matrices, que sospecho que serán mat1 y mat2 (ya que no se vuelven a usar en el programa) y es cuando salta el mensaje de error. Lo extraño es que ninguna de las dos matrices puede haber sido modificada previamente, ya que el operador de multiplicación y su parámetro son constantes y la asignación trabaja con la matriz resultado de la multiplicación (la que se obtiene del return). Por lo que me lleva a sospechar que la primera matriz que elimina la destructora (la que se elimina antes de hacer la asignación) es la que luego se vuelve a intentar eliminar y provoca el error. Entonces, he pensado en incluir en la función borrar() la misma condición que en la sobrecarga de asignación (y eliminarla de ésta), pero el error sigue ahí (todo esto ya está reflejado en el código de la clase incluido arriba). También he añadido la asignación matr = NULL, aunque no sé si al usar la operación delete un puntero queda con el valor NULL asignado o no.

También he encontrado esto buscando información sobre el delete:

Cuando se usa el operador delete con un puntero nulo, no se realiza ninguna acción. Esto permite usar el operador delete con punteros sin necesidad de preguntar si es nulo antes.

Fuente: http://c.conclase.net/curso/?cap=013b

Por lo tanto, no debería ser necesaria la condición if (matr != NULL);, ¿no?

¿Alguna idea de que es lo que puede estar ocurriendo?

Muchas gracias!

[amchacon]

¿Que código has usado para las pruebas?

PD: No sabía lo de los punteros null y delete. Siempre se aprende algo nuevo 

[xaps]

¿Que código has usado para las pruebas?

PD: No sabía lo de los punteros null y delete. Siempre se aprende algo nuevo 

El mismo que el del post principal, lo único que he cambiado ha sido la clase.
Te lo adjunto de todas formas:

Código (cpp) [Seleccionar] Expandir


#include "Matriz.h"

int main()
{
  int f1, c1, f2, c2;

  cout << "Introduce el tamaño de la primera matriz:" << endl;
  cin >> f1 >> c1;
  Matriz mat1(f1, c1);
  cout << "Introduce los valores de la matriz:" << endl;
  mat1.leer();

  cout << "Introduce el tamaño de la segunda matriz:" << endl;
  cin >> f2 >> c2;
  Matriz mat2(f2, c2);
  cout << "Introduce los valores de la matriz:" << endl;
  mat2.leer();


  if (c1 == f2)
  {
    Matriz res;
    res = mat1 * mat2;

    res.escribir();
  }
}


[amchacon]

Vale ya he arreglado el operador de asignación, el problema es que no devuelves una referencia sino un nuevo objeto matriz. Como no tienes hecho un constructor copia se copian los punteros literales, eso te provoca problemas...

Simplemente haz estos dos cambios:

- Haz que el operador = devuelva Matriz& en vez de un nuevo objeto.
- Create un constructor copia tal que así:

Código (cpp) [Seleccionar] Expandir

Matriz::Matriz(const Matriz &m)
{
    *this = m;
}

Con eso a mí me funciona perfecto.

[xaps]

Vale ya he arreglado el operador de asignación, el problema es que no devuelves una referencia sino un nuevo objeto matriz. Como no tienes hecho un constructor copia se copian los punteros literales, eso te provoca problemas...

Simplemente haz estos dos cambios:

- Haz que el operador = devuelva Matriz& en vez de un nuevo objeto.
- Create un constructor copia tal que así:

Código (cpp) [Seleccionar] Expandir

Matriz::Matriz(const Matriz &m)
{
    *this = m;
}

Con eso a mí me funciona perfecto.

¿A que te refieres con "Como no tienes hecho un constructor copia se copian los punteros literales"? No entiendo cual es el problema aún :S

[amchacon]

¿A que te refieres con "Como no tienes hecho un constructor copia se copian los punteros literales"? No entiendo cual es el problema aún :S

A ver, tu tenías definido el operador = para que devolviese un nuevo objeto matriz.

Como has hecho return *this esa matriz era una copia de this. Para hacer copias de un objeto se llama al constructor copia.

Si no has definido el constructor copia, el compilador crea uno por ti. El problema esque el compilador hace la copia literal. Lo que implica que se copian los punteros de la matriz tal cual.

Cuando esa matriz "copia" desaparece, se llama al destructor y se libera la memoria. Cuando la matriz "original" desaparece, se vuelve a llamar al destructor y se libera la misma memoria (¡Los punteros son iguales!). De ahí la excepción en tiempo de ejcución.

Aunque el error se solucionaría poniendo Matriz& en el retorno del operador =, no puedes dejarlo así porque ya sabes que las copias te van a fallar. Asi que te he rehecho el constructor copia (y para no copy&pastear el código, me he aprovechado del operador de asignación que hemos definido).

PD: Ya que estoy, los demás operadores (+ - *) también debería devolver Matriz& en vez de Matriz. De lo contrario estarás haciendo copias al montón