Проблемы с конструктором копирования и деструктором.

[Янус]

Код:

#include "stdafx.h"
#include <conio.h>
#include <iostream>

using namespace std;

class matrix;

class matrix {
int size;
	int **p;
public:
	

	//matrix(){};

	matrix(int n);

	~matrix();
	matrix(const matrix &a);

	int get (int i, int j);
	void put (int i, int j, int a);

	matrix operator+ (const matrix& ob1);
	matrix operator- (const matrix& ob1);
	matrix& operator= (const matrix& ob1);
	friend ostream & operator << (ostream &,const matrix &);
	friend istream & operator >> (istream &,const matrix &);
};

matrix::matrix(int n)
{
	p = new int *[n];

	if (p==NULL) {
		cout <<"No massiv";
		exit(1);
	}

	for (int i=0;i<n;i++)
	{
		p[i]=new int [n];

		if (p[i]==NULL)
		{
			cout <<"No massiv";
			exit(1);
		}

		for (int j=0;j<n;j++)	p[i][j]=0;
			
		size=n;	
	}
}
	matrix::~matrix()
	{
		for (int i=0;i<size;i++)
			delete p[i];
		delete [] p;
	}


	matrix::matrix(const matrix &a)
	{
        p=new int *[a.size];
	       if (p==0) {
		   cout <<"No massiv";
		      exit(1);
	}
	for (int i=0;i<a.size;i++)
	{
		p[i]=new int [a.size];
		if (p[i]==0)
		{
         cout <<"No massiv";
		 exit(1);
		 p[i]=p[a.size];
		}

		
		
	}
	}


	int matrix::get(int i,int j)//prrint
	{
		return p[i][j];
	}

	void matrix::put(int i,int j,int a)//vvod
	{
		p[i][j]=a;
		//return 0;
	}

matrix matrix::operator+ (const matrix& ob1)
{
	matrix temp(size);
	for(int i=0;i<size;i++)
	{
		for(int j=0;j<size;j++)
		{
			temp.p[i][j] = p[i][j] + ob1.p[i][j];
		}
	}
	return temp;
};
matrix matrix::operator-(const matrix& ob1)
{
matrix temp(size);
	for(int i=0;i<size;i++)
	{
		for(int j=0;j<size;j++)
		{
			temp.p[i][j] = p[i][j] - ob1.p[i][j];
		}
	}
	return temp;
};
matrix& matrix::operator= (const matrix& ob1)
{
	//matrix temp(size);
	for(int i=0;i<size;i++)
	{
		for(int j=0;j<size;j++)
		{
			p[i][j] = ob1.p[i][j];
		
		}
	}
	return *this;
};
 ostream & operator << (ostream & output,const matrix &a)
{
 for(int i=0;i<a.size;++i)
	{
		output << "Line" << (i+1) << ":";
		
		for(int j=0;j<a.size;++j)
		{
			output << "\t"<< a.p[i][j];
		}
		output  << endl;
	}
 return output;
};

istream & operator >> (istream & in,const matrix &a)
{
	for(int i=0;i<a.size;i++)
	{
		for(int j=0;j<a.size;j++)
		{
			cout << "Element" << (i+1) << " " << (j+1) << ":";
		    in >> a.p[i][j]; 
		}
	}
	return in;
}

int main ()
{
	int a;
	int n;
	cout <<"razmernost massiva:  ";
	cin >> n;
	matrix m1(n);
	cout << "Vvod massiva A: ";
	cout << "\n";

	cin >> m1;
	cout << "\n";
	cout << "Massiv A:";
	cout << "\n";

	cout << m1;

	matrix m2(n);
    cout << "\n";
	cout << "Vvod massiva B:";
    cout << "\n";

	cin >> m2; 
	cout << "\n";
	cout << "Massiv B:";
	cout << "\n";

	cout << m2;


	cout << "\n";
    cout << "A+B:";
    cout << "\n";

	
    cout << (m2+m1);
	
	cout << "\n";
	cout << "A-B:";
	cout << "\n";

	cout << (m1-m2);


	cout << "\n";
	cout << "B-A:";
	cout << "\n";

	cout << (m2-m1);

	getch();
return 0;
}

Имеется вышеуказанная программа, проблемы в ней следующие: 1. Можно ввести матрицы и они даже выведутся, но ни какие арифметические действия не выполняются.
2. Если отключить конструктор копирования (а следовательно и перегрузку оператора =) и деструктор то все заработает...
Мне понятно, что проблемы где-то в них но где не понимаю .
Прошу больно не пинать, это моя 2 программа на С++ в жизни... Заранее благодарен за ответы и подсказки.      

[lapulya]

Янус,

Код:

	p[i]=new int [a.size];
	if (p[i]==0)
	{
        	 cout <<"No massiv";
		 exit(1);
		 p[i]=p[a.size];
	}

вот это содержится внутри копирующего конструктора, и что же тут происходит, а происходит то,  что при нормальной работе программы (память выделилась успешно) копирование самой матрицы НЕ происходит, надо так

Код:

	p[i]=new int [a.size];
	if (p[i]==0)
	{
        	 cout <<"No massiv";
		 exit(1);
	}
	 p[i]=p[a.size];

Остальное ваще не смотрел, раз ты локализовал проблему))) то только в локализованной области я и смотрел

[Янус]

А как перегрузка оператора= и деструктор ? Правильно написал ?

[lapulya]

Янус, Оператор = для твоего приложения (это существенное уточнение) нормально, деструктор надо так

Код:

	matrix::~matrix()
	{
		for (int i=0;i<size;i++)
			delete []p[i];
		delete [] p;
	}

т.е. ты в обоих случаях удаляешь массивы, а массивы удаляются так delete []p

[Янус]

Болшое спасибо!

[lapulya]

Янус, неее у тебя еще ошибки в копирующем конструкторе надо так (привожу полный код копирующего конструктора)

Код:

matrix::matrix(const matrix &a)
{

	p=new int *[a.size];
	if (p==0)
	{
		cout <<"No massiv";
		exit(1);
	}

	for (int i=0;i<a.size;i++)
	{
		p[i]=new int [a.size];
		if (p[i]==0)
		{

			cout <<"No massiv";
			exit(1);
		}
		
		for (int j = 0 ; j < a.size ; ++j)
			p[i][j] = a.p[i][j];
	}
	
	size = a.size;
}

но не проверял, так что рекомендую проверить.

[RuNTiME]

Всем привет!, тут есть ошибка:
Создается локальный экземпляр класса matrix, переменная temp, которая затем возвращается,
но, как только переменная temp выходит за пределы видимости функции, вызывается ее деструктор.
При этом освобождается память, выделенная под переменные класса в том числе и temp.p. После чего
отрабатывает вывод результатов и выдает не верные данные, а то и вообще может вылететь с ошибкой
доступа к памяти.

Код:

matrix matrix::operator+ (const matrix& ob1)
{
	matrix temp(size);
	for(int i=0;i<size;i++)
	{
		for(int j=0;j<size;j++)
		{
			temp.p[i][j] = p[i][j] + ob1.p[i][j];
		}
	}
	return temp;
};
matrix matrix::operator-(const matrix& ob1)
{
matrix temp(size);
	for(int i=0;i<size;i++)
	{
		for(int j=0;j<size;j++)
		{
			temp.p[i][j] = p[i][j] - ob1.p[i][j];
		}
	}
	return temp;
};

Проблему решить можно, если создавать переменную temp через оператор new, но тогда придется вызывать каждый раз для нее delete руками, что несколько неудобно...

[Джон]

RuNTiME, в данном случае ты ошибаешься. Как ты себе представляешь

Создается локальный экземпляр класса matrix, переменная temp, которая затем возвращается,

Что именно возвращается? А возвращается копия объекта, а не указатель на него. Так что всё в порядке. Другой правомерный вопрос можно задать - а реализован ли конструктор копирования класса matrix, который будет в данном случае вызван по умолчанию? Может просто код приведён не полностью.

[RuNTiME]

Джон, да действительно, ошибка была в конструкторе копирования. Я переписал код по - своему, из этой темы . Просто забыл дописать sizeof(int) в конструкторе копирования

Код:

matrix::matrix(const matrix &a) {
	mem_size = a.mem_size;
	p = new int [mem_size];
	if (p == NULL) {
		cout << "No massiv";
		exit( 1 );
	}
	size = a.size;
	memcpy(p, a.p, mem_size * sizeof(int));
}

И вот собственно весь код полностью, несколько другой способ хранения матрицы в памяти:

Код:

// test_1.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "stdafx.h"
#include <conio.h>
#include <iostream>

using namespace std;

#define ELEMENT(p,i,j,n) ((p)[i + n * j])

class matrix {
	int size;
	int *p;
	unsigned int mem_size;
public:
	matrix(int n);

	~matrix();
	matrix(const matrix &a);

	int get (int i, int j);
	void put (int i, int j, int a);

	matrix operator+ (const matrix& ob1);
	matrix operator- (const matrix& ob1);
	matrix& operator= (const matrix& ob1);
	friend ostream & operator << (ostream &, const matrix &);
	friend istream & operator >> (istream &, const matrix &);
};

matrix::matrix(int n) {
	mem_size = n * n;
	p = new int [mem_size];
	if (p == NULL) {
		cout << "No massiv";
		exit( 1 );
	}
	memset(p, 0, mem_size * sizeof(int));
	size=n;
}

matrix::~matrix() {
	delete [] p;
}

matrix::matrix(const matrix &a) {
	mem_size = a.mem_size;
	p = new int [mem_size];
	if (p == NULL) {
		cout << "No massiv";
		exit( 1 );
	}
	size = a.size;
	memcpy(p, a.p, mem_size * sizeof(int));
}

int matrix::get(int i, int j) { //prrint
	return ELEMENT(p, i, j, size);
}

void matrix::put(int i, int j, int a) { //vvod
	ELEMENT(p, i, j, size) = a;
}

matrix matrix::operator+ (const matrix& ob1) {
	matrix temp(size);
	for(int i = 0; i < size; i++) {
		for(int j = 0; j < size; j++) {
			ELEMENT(temp.p, i, j, size) = 
				ELEMENT(p, i, j, size) + ELEMENT(ob1.p, i, j, size);
		}
	}
	return temp;
}

matrix matrix::operator- (const matrix& ob1) {
	matrix temp(size);
	for(int i = 0; i < size; i++) {
		for(int j = 0; j < size; j++)
			ELEMENT(temp.p, i, j, size) =
				ELEMENT(p, i, j, size) - ELEMENT(ob1.p, i, j, size);
	}
	return temp;
}

matrix& matrix::operator= (const matrix& ob1) {
	//matrix temp(size);
	for(int i = 0; i < size; i++) {
		for(int j = 0; j < size; j++)
			ELEMENT(p, i, j, size) = ELEMENT(ob1.p, i, j, size);
	}
	return *this;
}

ostream & operator << (ostream & output, const matrix &a) {
	for(int i = 0; i < a.size; ++i) {
		output << "Line" << (i + 1) << ":";
		for(int j = 0; j<a.size; ++j)
			output << "\t"<< ELEMENT(a.p, i, j, a.size);
		output << endl;
	}
	return output;
}

istream & operator >> (istream & in, const matrix &a) {
	for(int i=0; i<a.size; i++) {
		for(int j=0; j<a.size; j++) {
			cout << "Element" << (i + 1) << " " << (j + 1) << ":";
		    in >> ELEMENT(a.p, i, j, a.size); 
		}
	}
	return in;
}

int _tmain(int argc, _TCHAR* argv[]) {
	int n;
	cout <<"razmernost massiva:  ";
	cin >> n;
	matrix m1( n );
	cout << "Vvod massiva A: ";
	cout << "\n";

	cin >> m1;
	cout << "\n";
	cout << "Massiv A:";
	cout << "\n";

	cout << m1;

	matrix m2( n );
    cout << "\n";
	cout << "Vvod massiva B:";
    cout << "\n";

	cin >> m2; 
	cout << "\n";
	cout << "Massiv B:";
	cout << "\n";

	cout << m2;

	cout << "\n";
    cout << "A+B:";
    cout << "\n";

    cout << (m2 + m1);
	
	cout << "\n";
	cout << "A-B:";
	cout << "\n";

	cout << (m1 - m2);

	cout << "\n";
	cout << "B-A:";
	cout << "\n";

	cout << (m2 - m1);

	getch();

	return 0;
}

[npak]

Ошибка в арифметичесих операторах и операторе присваивания.

Вы не проверяете, что размер аргумента ob1.size равен this->size, поэтому рискуете получить странные результаты, если this->size > ob1.size.

Если класс matrix сделать шаблоном, то проверку совместимости матриц компилятор будет производить автоматически.

Код:

template<unsigned int size> class matrix {
                 int * p;
                 const static unsigned int mem_size = size*size*sizeof(int);
public:
                 int get (int i, int j);
                 void put (int i, int j, int a);
                 matrix operator+ (const matrix& ob1);
                 matrix operator- (const matrix& ob1);
                 matrix& operator= (const matrix& ob1);
};

[npak]

Есть также замечания по поводу копирующего конструктора и оператора присваивания. Они очень неэффективны. Специально для таких случаев как массивы, матрицы, строки, придуман приём под названием copy-on-write, "копирование при записи". Посмотрите в учебниках, это должно быть описано.

[npak]

Ошибка в декларации оператора istream & operator >> (istream &, const matrix &);
Как вы собираетесь писать в константный объект? Должно быть

Код:

friend istream & operator >> (istream &, matrix &);

[Вад]

npak, есть ли смысл в общем случае задавать размер матрицы в качестве параметра шаблона? Мне кажется, это негибко. Как быть, когда размер задаётся во время выполнения?

[npak]

Да, согласен, погорячился. Это имеет смысл только для ситуаций, в которых размеры известны на этапе компиляции.

В динамическом случае необходимо проверять равенство размеров в операторах и выбрасывать исключение в случае несовпадения размеров матриц.

[Янус]

Господа, извините за тупость но, нельзя ли поподробнее об несколько ином методе создания двумерного динамического массива...

[Алексей++]

Янус, о каком именно методе нужно рассказать, уточни )

[Янус]

Я программировал создание динамического массива как описано у Подбельсокого в "Стандартный Си++", вышло довольно громоздко но зато мне понятно) и как я понял (из преведенного здесь выше примера) довольно неуместно. Не могли бы Вы объяснить(или дать ссылку на место где это описывается), как создавать динамические массивы в С++ наиболее оптимальным способом.По типу того как это зделал RuNTiME (насколько я могу понять более эффективно, но мне не понятно) в пределанной моей прграмме. Спасибо за отклик.

[Алексей++]

Янус, да вот все любят разные "школы" ) Но лично мне ближе пример - инкапсулирование массива в классе, в конструкторе создаём через new, в деструкторе мочим. Красиво и ничего лишнего. Только форматирование кода у RuNTiME местами некрасивое )

[Finch]

Янус, В С++ нет понятия многомерные массивы. Есть только одномерный массив. А все остальное это интерпретация программиста. Я пока что знаю два способа интерпретации 2 мерного массива.

Первый способ. Создается 1 одномерный массив, но размерностью ширина*высота нужного нам двух мерного массива. И потом программно, путем не хитрой математики, нарезается он на нужный нам двухмерный массив.
Пример:
Нам нужна матрица 4*16 скажем. Создаем одномерный массив размерностью 64 элемента. Теперь, чтобы получить доступ скажем к полю {3, 2} Нужно только записать array[3*16+2].

Второй способ. Создаем массив строк. Т.е. создаем 4 раза массивы с 16 элементами. И сохраняем указатели на эти массивы, в массиве указателей.
Тогда доступ к ним осушествляется уже как array[3][2]. Т.е при выполнении кода, сначало возьмется указатель на 3 строку, а затем в строке возьмется 2 элемент.

[RuNTiME]

npak, Добрый день! Вы писали:

Ошибка в декларации оператора istream & operator >> (istream &, const matrix &);
Как вы собираетесь писать в константный объект?

Проблемы тут нет, здесь объявлена константная ссылка, а не константный объект, по этому сам объект может быть изменен. Проблем с записью в объект не возникает.

[Алексей++]

RuNTiME, ссылка - это всегда константный объект, на самом деле Поскольку ссылку поменять нельзя после инициализации. А константная ссылка не позволит менять и объект, на который она ссылается. Так что тебе правильно говорят

[RuNTiME]

Алексей1153++,

RuNTiME, ссылка - это всегда константный объект, на самом деле Улыбаюсь Поскольку ссылку поменять нельзя после инициализации. А константная ссылка не позволит менять и объект, на который она ссылается. Так что тебе правильно говорят

Ссылка, да, это константный объект:) Но! переменная передаваемая по ссылке может быть изменена. Что есть факт.

Хммм... И вот тогда, если по вашему, ссылка не дает менять объектную переменную, получается парадокс. Код, который я написал, работает:) и в том числе работает:

Код:

matrix m1( n );
	cout << "Vvod massiva A: ";
	cout << "\n";

	cin >> m1; // <=== Запись в объект !!!
	cout << "\n";
	cout << "Massiv A:";
	cout << "\n";

Чем сможете объяснить такого вида парадокс?
Дело в другом, возможно const можно и не писать, т.к. ссылка и есть константа. Но это уже дело стиля и думаю, что явно показать константность, есть проявление хорошего стиля. Да и кстати, как раз эти строки кода я вообще - то не трогал:

Код:

	friend ostream & operator << (ostream &, const matrix &);
	friend istream & operator >> (istream &, const matrix &);

А Янус, как он писал выше, делал по примеру из учебника... не думаю, что в учебнике по программированию напишут не правильно... хотя бывает и такое, но только не в этот раз:)

[RuNTiME]

Янус,  Твой способ размещения не очень эффективен по нескольким причинам:
1) Многократно вызывается выделение памяти. При этом каждый раз системой кроме самой памяти размещается некая структура описывающая размер выделенной памяти  и еще некоторые другие системные параметры. Что приводит к большим потерям памяти.
2) Доступ к монолитному блоку происходит всегда быстрее, чем через редиректы по указателям. К примеру нужно обратится к ячейке [2][5], процессору нужно сначала вычислить указатель на элемент [2], прочитать указатель в ячейке [2] на массив, в котором содержится элемент [5], вычислить смещение на элемент [5] и только после этого прочитать или записать его значение.
    Если мы выделяем память одним блоком, получаем монолитный блок, который разбивается на равные отрезки. Причем получение указателя на элемент, сводится к двум операциям сложить и умножить. Что выполняется гораздо быстрее.
3) Освобождение памяти. В твоем случае, приходится многократно вызывать delete, что выполняется дольше, чем освобождение такого же количества памяти, но выделенного одним блоком и удаляемого соответственно однократным вызовом delete.

[Алексей++]

RuNTiME, никаких парадоксов:

Код:

int i=0;
int& i1=i;
const int& i2=i;

i1=1; //можно
i2=1; //нельзя

[RuNTiME]

Алексей1153++,  Да в этом я был не прав, константная ссылка не дает изменять объект. Но в данном коде это и не требуется, сам объект не изменяется. А изменяются данные находящиеся по указателю p, которые для компилятора не относятся к самому объекту.

Т.е.

Код:

a.p = NULL;

вызывает ошибку при компиляции и это правильно, т.к. пытаемся изменить сам объект.

А вот следующий код допустим, т.к. изменяются данные по указателю p, а сам указатель
в этом случае только считывается:

Код:

a.p[1] = 5;

А раз при записи данных, сам объект не изменяется и это не требуется, то пусть лучше будет объявлен как константный. Избавит от случайных ошибок, что уже Гуд .

Код:

istream & operator >> (istream & in, const matrix &a) {
	for(int i=0; i<a.size; i++) {
		for(int j=0; j<a.size; j++) {
			cout << "Element" << (i + 1) << " " << (j + 1) << ":";
		    in >> ELEMENT(a.p, i, j, a.size); 
		}
	}
	return in;
}

[Алексей++]

это всё понятно, а что тогда делает
in >> ELEMENT(a.p, i, j, a.size);
?

это не может скомпилироваться по идее, так как a - константный объект, а "p" принадлежит а

[Алексей++]

и вообще бы я на твоём месте не шутил вот так с макросом
#define ELEMENT(p,i,j,n) ((p)[i + n * j])
а сделал бы "p" приватным , а макрос - инлайн методом

[RuNTiME]

Алексей1153++,  Это скомпилируется без проблем ELEMENT обявлен, как

Код:

#define ELEMENT(p,i,j,n) ((p)[i + n * j])

что раскрывается препроцессором таким образом

Код:

p[i + n * j] = value; // какое - то значение

что тем самым не противоречит и такому варианту:

Код:

p[1] = 5;

Так что все компилится проверено

[Алексей++]

RuNTiME, ага, канает, я проверил.

Код:

	struct matrix
	{
		BYTE* p;
	};


	matrix M;

	const matrix& const M2=M;

	M2.p[0]=0;

Но ты же должен понимать, что это косяк ? Всё таки метод лучше сделать, тогда таких накладок не будет

[RuNTiME]

Алексей1153++,  inline это конечно круто, но как всегда есть одно но к примеру в майкрософтовском компиляторе есть такая фича, что он оставляет за собой выбор, делать или нет функцию inline'овой. По этому может получиться так, что не каждая инлайн функция будет действительно инлайн. А так, конечно правильно, инлайн фукнции имеют большие преимущества перед макросами. Хотя это опять же зависит от ситуации.