Irritating heap corruption error in VC++2005

[qxcdz]

'kay, I've been doing coasting along in C++, overloading operators, defining matrix arithmetic... Except, there's this error stopping my otherwise flawless addition function from working. When the function ends, it calls ~matrix(), probably for the temp variable inside the function, and the delete [] data_ corrupts the heap. How, I don't know, so I'm showing you guys in the hope that someone will spot what I've done wrong.

HelloWorld.cpp:

#include "stdafx.h"
#include <iostream>
#include <string>
#include "matrix.h"

using namespace std;
typedef unsigned char byte;

int main(int argc, _TCHAR* argv[])
{
	matrix tst1(5,5);
	matrix tst2(5,5);
	matrix tst3(3,6);
	tst1(3,4)=56;
	tst3=tst2+tst1;
	int tst=tst3(3,4);
	cout << tst;

	return 0;
};

stdafx.h:

// stdafx.h : include file for standard system include files,
// or project specific include files that are used frequently, but
// are changed infrequently
//

#pragma once


#define WIN32_LEAN_AND_MEAN		// Exclude rarely-used stuff from Windows headers
#include <stdio.h>
#include <tchar.h>



// TODO: reference additional headers your program requires here

matrix.h:

#define MATRIX_TYPE unsigned char

using namespace std;
//typedef unsigned char byte; --no longer needed due to #define directive

class matrix
{
	MATRIX_TYPE* data_; //using underscores to distinguish internal variables -- worth changing the rest of the code?
	int rows_,cols_;
public:
	matrix(int rows, int cols); //constructor
	~matrix(); //destructor DUMMY

	MATRIX_TYPE& operator() (int row, int col); //subscript operator
	MATRIX_TYPE  operator() (int row, int col) const; // subscript operator for constants
	matrix operator+(matrix op1); //matrix addition
	matrix& operator=(const matrix& op1); //assignment of a matrix, to account for size differences
};

matrix::matrix(int rowin, int colin)
{
	if(rowin==0){rows_=1;}else{rows_=rowin;};
	if(colin==0){cols_=1;}else{cols_=rowin;};
	data_=new MATRIX_TYPE[(rows_*cols_)-1];
};

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

MATRIX_TYPE& matrix::operator() (int row, int col)
{
	if(row<rows_ && col<cols_)
	{
		return data_[(cols_*row)+col];
	}else{
		cout << "index out of range for this array" << endl;
		return data_[0];
	};
};

MATRIX_TYPE  matrix::operator() (int row, int col) const
{
	if(row<rows_ && col<cols_)
	{
		return data_[(cols_*row)+col];
	}else{
		cout << "index out of range for this array" << endl;
		return data_[0];
	};
};

matrix& matrix::operator=(const matrix& op1)
{
	if(this != &op1) //must not be the same object!
	{
		delete[] data_; //get rid of the old data
		rows_=op1.rows_; // make this new matrix have the same dimensions as the old one
		cols_=op1.cols_;
		data_=new MATRIX_TYPE[(rows_*cols_)-1]; //reassign space for data

		for(int r=0;r<rows_;r++)
		{
			for(int c=0;c<cols_;c++)
			{
				data_[(cols_*r)+c]=op1(r,c); //copy data over from other matrix
			};
		};

	};
	return *this;
};

matrix matrix::operator +(matrix op1)
{
	if((op1.rows_==rows_)&&(op1.cols_==cols_))
	{
		matrix result(rows_,cols_);
		for(int r=0;r<rows_;r++)
		{
			for(int c=0;c<cols_;c++)
			{
				result(r,c)=op1(r,c)+data_[(cols_*r)+c]; // fill result matrix with sums
			};
		};
		return result; // this line keeps causing a heap corruption!
	}else{
		cout << "matricies are not of equal dimension" << endl;
		matrix ret(1,1);
		return ret;
	};
};

 

[Mark44]

The OP hasn't been around in years, but others might benefit from my analysis of this code.

The most obvious problem is one statement in main():

int main(int argc, _TCHAR* argv[])
{
    matrix tst1(5,5);
    matrix tst2(5,5);
    matrix tst3(3,6);
    tst1(3,4)=56;
    tst3=tst2+tst1;    // <---- Problem here
    int tst=tst3(3,4);
    cout << tst;

    return 0;
};

In the line with the comment you are 1) adding two arrays of the same size (that's fine), but 2) attempting to store the contents of a 5 x 5 matrix in a 3 x 6 matrix. This won't work.

There is also a problem in the line that follows, where you are attempting to read the value in tst3(3, 4), but the tst3 array has only 3 rows that have indexes of 0, 1, and 2.

The less obvious, but more difficult problems are in your constructor and in your addition operator code.
First, the constructor:

matrix::matrix(int rowin, int colin)
{
    if (rowin == 0) { rows_ = 1; }
    else { rows_ = rowin; };
    if (colin == 0) { cols_ = 1; }
    else { cols_ = rowin; };
    data_ = new MATRIX_TYPE[(rows_*cols_) - 1];  // <=== Problem here
};

In the commented line, your heap matrix, data_, is too small. Inside the brackets should be rows_ * cols_. The same problem is present in your assignment operator code.

In your addition operator:

matrix matrix::operator +(matrix op1)
{
    if ((op1.rows_ == rows_) && (op1.cols_ == cols_))
    {
        matrix result(rows_, cols_);              // <=== Problem here
        for (int r = 0; r < rows_; r++)
        {
            for (int c = 0; c < cols_; c++)
            {
                result(r, c) = op1(r, c) + data_[(cols_*r) + c]; // fill result matrix with sums
            };
        };
        return result; // this line keeps causing a heap corruption!
    }
    else {
        cout << "matricies are not of equal dimension" << endl;
        matrix ret(1, 1);
        return ret;
    };
};

Your result variable is an automatic variable (allocated on the stack) that goes out of scope as soon as this function returns. When result goes out of scope, the matrix destructor is called before the contents of the array sum can be copied over to the tst3 array in main(). One fix for this is to declare result to be static.

BTW, by making the changes I described, I was able to get the code to work.