dwelly@saddle-lk.cs.UAlberta.CA (Andrew Dwelly) (03/14/91)
I've just started using c++ and I have an example of code here that
I can't get to run, even though it seems to be legal c++. This is almost
certainly a beginners mistake, but I can't for the life of me see what the
problem is.
The situation :
I've produced a class of dynamic matrices, with appropriate access, constructor
and destructor functions. Now I'm trying to get a simple matrix multiply to
work, overloading the * operator. In my main function, I create and print
out a matrix "m", then try to print out m*m;
What I think I'm doing :
I create an automatic matrix variable for my results, then perform
a simple multiply on the inputs, placing the result in the output
variable. Finally I return "out".
Since my function has been declared as returning a matrix, I understood that
a _copy_ of "out" was returned before the destructor was called on "out".
What actually happens :
Compiling with g++ on a Sparc produces the following results :-
saddle-lk% Matrix
1 2
3 4
2.24208e-44 2.16332e-40
0 2.16355e-40
Segmentation fault (core dumped)
Would someone take pity on me and tell me what I'm doing wrong ???
Andy Dwelly
-------------------------------------------------------------------------------
#include <stream.h>
#include <stddef.h>
#include <bool.h>
class matrix
{
float **m;
int rows;
int cols;
public:
matrix(int,int);
~matrix();
float*& operator[](int);
int sizer();
int sizec();
friend matrix
operator*(matrix&, matrix&);
};
// We want to be able to print it out
ostream& operator<<(ostream& s, matrix& m) // m must be a reference, why ?
{
for (int i = 0; i < m.sizer(); i++)
{
for (int j = 0; j < m.sizec(); j++)
{
s << m[i][j] << " ";
}
s << "\n";
}
return s;
}
matrix::matrix(int r, int c)
{
rows = r;
cols = c;
m = new float*[r];
for (int i = 0; i < r; i++)
{
m[i] = new float[c];
}
}
matrix::~matrix()
{
for (int i = 0; i < rows; i++)
{
delete m[i];
}
delete m;
}
float*&
matrix::operator[](int i)
{
return m[i];
}
matrix::sizer()
{
return rows;
}
matrix::sizec()
{
return cols;
}
// Problems here ????
matrix
operator*(matrix& a, matrix& b)
{
matrix out(a.sizer(), b.sizec());
if (a.sizec() == b.sizer())
{
for (int i = 0; i < a.sizer(); i++)
{
float *row = a.m[i];
float multsum;
for (int j = 0; j < b.sizec(); j++)
{
multsum = 0.0;
for (int k = 0; k < b.sizer(); k++)
{
multsum += row[k] * b.m[k][j];
}
out.m[i][j] = multsum;
}
}
}
return out;
}
main()
{
matrix m(2,2);
m[0][0] = 1.0;
m[0][1] = 2.0;
m[1][0] = 3.0;
m[1][1] = 4.0;
cout << m << "\n";
cout << m * m << "\n";
}
--
******************************************************************************
Andy Dwelly : dwelly@cs.uablberta.ca, Tel: 403-492-7591
!@#$%$#, %^&*%, - Listen, who swears ? Christopher Robin has fallen downstairs.mittle@blinn.watson.ibm.com (Josh Mittleman) (03/15/91)
The problem is that you haven't implemented the copy constructor,
matrix::matrix(const matrix&)
When your multiplication routine return out, what is returned is a
field-by-field copy of out. It copies the value of each field; in
particular, it copies the value of the pointer out.m. So, we have a new
matrix which points to the same double array of floats as out. When you
exit matrix::operator*, out goes out of scope, and the destructor runs,
deleting the data now used by out and the return matrix.
Your lucky, by the way. This kind of error can be a royal pain to
discover.
You can solve your problem by adding matrix::matrix(const matrix&):
matrix::matrix(const matrix& M)
{
rows = M.rows;
cols = M.cols;
m = new float*[rows];
for (i = 0; i < rows; i++)
{
m[i] = new float[cols];
for (int j = 0; j < cols; j++)
{
m[i][j] = M[i][j];
}
}
}
You should also implement matrix::operator=(const matrix&), which will copy
the target matrix element by element, and will all deal with dimension
mismatch in some way. Otherwise, a statement like:
matrix m(2,2), n(2,2);
...
m = n;
will copy field-by-field, not element-by-element.
You also asked:
ostream& operator<<(ostream& s, matrix& m) // m must be a reference, why ?
It doesn't have to be a reference, but it prevents an unnecessary copy
operation. If you pass an object by value, then the function has to make a
temporary copy of the object. With large objects, that wastes time.
Actually, I recommend changing it to:
ostream& operator<<(ostream& s, const matrix& m)
^^^^^
This prevent you from accidently changing m within the function. Whenever
your function shouldn't change the value of its argument, make the argument
a const &. Whenever the function shouldn't change the value of the calling
object (*this), make the function const, if your version of C++ supports
that.
===========================================================================
Josh Mittleman (mittle@ibm.com or joshua@paul.rutgers.edu)
J2-C28 T.J. Watson Research Center, PO Box 704, Yorktown Heights, NY 10598mvm@jedi.harris-atd.com (Matt Mahoney) (03/15/91)
In article <1991Mar13.170912.4020@cs.UAlberta.CA> dwelly@saddle-lk.cs.UAlberta.CA (Andrew Dwelly) writes: > >Would someone take pity on me and tell me what I'm doing wrong ??? > >class matrix >{ > float **m; > int rows; > int cols; >public: > > matrix(int,int); > ~matrix(); > float*& operator[](int); > int sizer(); > int sizec(); > friend matrix > operator*(matrix&, matrix&); >}; > code deleted ... The * function has to copy a matrix when it returns, but there is no copy constructor. The default copy constructor just copies the contents, including pointers. The segmentation fault probably occurs when the destructors for the original (inside operator*) and the copy (cout << m * n) delete the same memory. To fix this, add: matrix(const matrix&); // Copy constructor matrix& operator=(const matrix&); // Assignment to your class. Both functions should allocate new memory when they copy their arguments. Also, you may want to change operator* to: friend matrix operator*(const matrix&, const matrix&); so you can pass temporary expressions, as in a * b * c. ----------------------------------------- Matt Mahoney, 407-727-5431, mvm@epg.harris.com #include <disclaimer.h>