I'm using DevC++ 4.9.9.2 on Windows XP. I've started using GSL for linear algebra, but I tried to write a matrix class a while ago, and I am had trouble getting matrix multiplication to work correctly.
Code:
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float* [n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
} ;
matrix& operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) {
sum=sum+A.MatrixArray[row][s]* B.MatrixArray[s][column];
}
C.MatrixArray[row][column]=sum;
}
}
return C;
}
void display(matrix& A)
{
int n=A.rows;
int m=A.columns;
for (int row=0; row< n; row++){
for(int column=0; column < m; column++){
cout << A.MatrixArray[row][column] << ' ';
}
cout << endl;
}
}
Okay. Actually, I guess I needed clarification for both. So, I think I finally have it. By the way, I have Practical C++ Programming by Oualline. Would you recommend getting another book?
Thanks,
Chris
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float* [n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
matrix operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix OutMatrix(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) {
sum=sum+A.MatrixArray[row][s]*B.MatrixArray[s][column];
}
OutMatrix.MatrixArray[row][column]=sum;
}
}
return OutMatrix;
}
matrix add( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
n=A.rows;
m=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
I figured out what was going on. Although it compiles, it kept warning me that I was declaring a reference to the local variable 'C' in my definition of matrix multiplication. If I declare C 'static,' then everything works fine, and I no longer get the warning.
When declaring a function that outputs a floating point number, this generally isn't necessary, right? So I don't understand why it is necessary here.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
All variables have two attributes - scope and lifetime. Normal 'auto' variable (an non-static local variable) has scope within a function, but also a lifetime that lasts only for the duration of the function execution, so you cannot return either a reference nor a pointer because the object does not exist after the function returns. The memory it used may be reused by something else, and its destructor will hav
In your case calling the destructor will delete the MatrixArray as well, so that would not exist either. You need to implement the copy constructor and the assignment operator because a default shallow copy is insufficient.
Your problem is fundamental and your solution (reference to static object) flawed. You should not return a reference but rather an object:
matrix operator * ( matrix& A, matrix& B) ;
Your solution of using a static changes lifetime to that of the program execution. The problem is that there is only one instance shared by all - they'll all have the same MatrixArray unless you implement the assignment operator and do a deep copy. Returning an object resolves this.
> When declaring a function that outputs a floating point
> number, this generally isn't necessary, right?
That is not the same thing. You's have teh same problem if you returned a reference to the float rather than a copy (object instance).
Generally with overloading arithmetic operators is is simpler to implement the assignment form (=) and then implement the * in terms of =. You should also implement the copy constructor and assignment operators for classes that need deep copy.
Clifford
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
So if I return an object instead of a reference, that should fix the problem? I tried that, but I still got junk. Perhaps I am still missing something. I don't quite understand what you're suggesting at the very end:
"Generally with overloading arithmetic operators is is simpler to implement the assignment form (=) and then implement the * in terms of =. You should also implement the copy constructor and assignment operators for classes that need deep copy. "
Also, if I comment out the destructor in the matrix class, then everything works fine (see below). Is this also a flawed solution, and why does it work in the first place?
Sorry,
Chris
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float[n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
/
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
*/
matrix operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix OutMatrix(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) {
sum=sum+A.MatrixArray[row][s]*B.MatrixArray[s][column];
}
OutMatrix.MatrixArray[row][column]=sum;
}
}
return OutMatrix;
}
matrix add( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
n=A.rows;
m=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
> So if I return an object instead of a reference, that should
> fix the problem? I tried that, but I still got junk.
Not exactly. You should return an object, but that is not the only problem with the code.
> Also, if I comment out the destructor in the matrix class, then everything works fine (see below).
Ouch! Because it does not delete the allocated memory, the data remains (and constitutes a severe memory leak!). However the class members and the MatrixArray pointer itself exist on the stack, and the memory they occupy may get later reused.
> Is this also a flawed solution, and why does it work in the first place?
Yes it is, and more by luck than judgement. It is relying on never releasing memory, and that the memory used by the local object itself is never reused after it is reclaimed.
> I don't quite understand what you're suggesting at the very end:
Which part? I said two things there.
Note that a matrix class is a common topic in C++ and there are many existing implementations (usually template based allowing you to create matrices of different types).
Also get yourself a good text on C++ that covers robust class design and operator overloading.
Clifford
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I'm using DevC++ 4.9.9.2 on Windows XP. I've started using GSL for linear algebra, but I tried to write a matrix class a while ago, and I am had trouble getting matrix multiplication to work correctly.
Code:
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float* [n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
} ;
matrix& operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) { sum=sum+A.MatrixArray[row][s]* B.MatrixArray[s][column]; } C.MatrixArray[row][column]=sum; }}
return C;
}
void display(matrix& A)
{
int n=A.rows;
int m=A.columns;
for (int row=0; row< n; row++){
for(int column=0; column < m; column++){
cout << A.MatrixArray[row][column] << ' ';
}
cout << endl;
}
}
int main(int argc, char *argv[])
{
matrix A(2,2);
matrix B(2,2);
A.MatrixArray[0][0]=1;
A.MatrixArray[0][1]=2;
A.MatrixArray[1][0]=3;
A.MatrixArray[1][1]=1;
B.MatrixArray[0][0]=4;
B.MatrixArray[0][1]=7;
B.MatrixArray[1][0]=1;
B.MatrixArray[1][1]=2;
display(A);
cout << endl;
display(B);
cout << endl;
matrix D(2,2);
D=B*A;
display(D);
}
Compile Log:
Compiler: Default compiler
Building Makefile: "C:\Dev-Cpp\Getting_Started\Matrix1\Makefile.win"
Executing make...
make.exe -f "C:\Dev-Cpp\Getting_Started\Matrix1\Makefile.win" all
g++.exe -c MatrixMain1.cpp -o MatrixMain1.o -I"C:/Dev-Cpp/lib/gcc/mingw32/3.4.2/include" -I"C:/Dev-Cpp/include/c++/3.4.2/backward" -I"C:/Dev-Cpp/include/c++/3.4.2/mingw32" -I"C:/Dev-Cpp/include/c++/3.4.2" -I"C:/Dev-Cpp/include" -I"C:/Dev-Cpp/joshi"
MatrixMain1.cpp: In function
matrix& operator*(matrix&, matrix&)': MatrixMain1.cpp:42: warning: reference to local variableC' returnedg++.exe MatrixMain1.o -o "Matrix1.exe" -L"C:/Dev-Cpp/lib"
Execution terminated
Compilation successful
Okay. Actually, I guess I needed clarification for both. So, I think I finally have it. By the way, I have Practical C++ Programming by Oualline. Would you recommend getting another book?
Thanks,
Chris
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float* [n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
matrix(const matrix& A):rows(A.rows),columns(A.columns){
MatrixArray = new float* [A.rows]; for (int i = 0; i < A.rows; i++) { MatrixArray[i] = new float [A.columns]; }for (int i=0; i< A.rows; i++){
for(int j=0; j< A.columns; j++){
MatrixArray[i][j]=A.MatrixArray[i][j];
}
}
}
} ;
matrix operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix OutMatrix(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) { sum=sum+A.MatrixArray[row][s]*B.MatrixArray[s][column]; } OutMatrix.MatrixArray[row][column]=sum; }}
return OutMatrix;
}
matrix add( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
n=A.rows;
m=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
}
return C;
}
void display(matrix& A)
{
int n=A.rows;
int m=A.columns;
for (int row=0; row< n; row++){
for(int column=0; column < m; column++){
cout << A.MatrixArray[row][column] << ' ';
}
cout << endl;
}
}
int main(int argc, char *argv[])
{
matrix A(2,2);
matrix B(2,2);
A.MatrixArray[0][0]=1.0;
A.MatrixArray[0][1]=2.0;
A.MatrixArray[1][0]=3.0;
A.MatrixArray[1][1]=1.0;
B.MatrixArray[0][0]=3.0;
B.MatrixArray[0][1]=4.0;
B.MatrixArray[1][0]=1.0;
B.MatrixArray[1][1]=2.0;
display(A);
cout << endl;
display(B);
cout << endl;
matrix D=add(A,B);
display(D);
cout << endl;
matrix E=BA;
matrix F=AB;
display(E);
cout << endl;
display(F);
system("PAUSE");
return EXIT_SUCCESS;
}
I figured out what was going on. Although it compiles, it kept warning me that I was declaring a reference to the local variable 'C' in my definition of matrix multiplication. If I declare C 'static,' then everything works fine, and I no longer get the warning.
When declaring a function that outputs a floating point number, this generally isn't necessary, right? So I don't understand why it is necessary here.
All variables have two attributes - scope and lifetime. Normal 'auto' variable (an non-static local variable) has scope within a function, but also a lifetime that lasts only for the duration of the function execution, so you cannot return either a reference nor a pointer because the object does not exist after the function returns. The memory it used may be reused by something else, and its destructor will hav
In your case calling the destructor will delete the MatrixArray as well, so that would not exist either. You need to implement the copy constructor and the assignment operator because a default shallow copy is insufficient.
Your problem is fundamental and your solution (reference to static object) flawed. You should not return a reference but rather an object:
matrix operator * ( matrix& A, matrix& B) ;
Your solution of using a static changes lifetime to that of the program execution. The problem is that there is only one instance shared by all - they'll all have the same MatrixArray unless you implement the assignment operator and do a deep copy. Returning an object resolves this.
> When declaring a function that outputs a floating point
> number, this generally isn't necessary, right?
That is not the same thing. You's have teh same problem if you returned a reference to the float rather than a copy (object instance).
Generally with overloading arithmetic operators is is simpler to implement the assignment form (=) and then implement the * in terms of =. You should also implement the copy constructor and assignment operators for classes that need deep copy.
Clifford
I managed ot post without completing the end of the first paragraph:
[...] and its destructor will have been called.
So if I return an object instead of a reference, that should fix the problem? I tried that, but I still got junk. Perhaps I am still missing something. I don't quite understand what you're suggesting at the very end:
"Generally with overloading arithmetic operators is is simpler to implement the assignment form (=) and then implement the * in terms of =. You should also implement the copy constructor and assignment operators for classes that need deep copy. "
Also, if I comment out the destructor in the matrix class, then everything works fine (see below). Is this also a flawed solution, and why does it work in the first place?
Sorry,
Chris
include <cstdlib>
include <iostream>
include<cmath>
using namespace std;
class matrix {
public:
int rows;
int columns;
float** MatrixArray;
matrix (int n =0, int m =0) : rows(n), columns(m) {
MatrixArray = new float [n];
for (int i = 0; i < n; i++) {
MatrixArray[i] = new float [m];
}
}
/
~matrix() {
for (int i = 0; i < rows; i++) {
delete[] MatrixArray[i];
}
delete[] MatrixArray;
}
*/
/*
matrix(&matrix A):rows(A.rows),columns(A.columns){
MatrixArray = new float* [A.rows]; for (int i = 0; i < A.rows; i++) { MatrixArray[i] = new float [A.columns]; } }for (int i=0; i<A.rows; i++){
for(int j=0; j<A.columns; j++){
MatrixArray[i][j]:=A.MatrixArray[i][j];
}
}
}
*/
} ;
matrix operator * ( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
int p;
n=A.rows;
m=B.columns;
p=A.columns;
matrix OutMatrix(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
float sum=0;
for (int s=0; s < p; s++) { sum=sum+A.MatrixArray[row][s]*B.MatrixArray[s][column]; } OutMatrix.MatrixArray[row][column]=sum; }}
return OutMatrix;
}
matrix add( matrix& A, matrix& B) // We multiply two matrices
{
int n;
int m;
n=A.rows;
m=A.columns;
matrix C(n,m);
for (int row=0; row < n; row++){
for (int column=0; column < m; column ++){
}
return C;
}
void display(matrix& A)
{
int n=A.rows;
int m=A.columns;
for (int row=0; row< n; row++){
for(int column=0; column < m; column++){
cout << A.MatrixArray[row][column] << ' ';
}
cout << endl;
}
}
int main(int argc, char *argv[])
{
matrix A(2,2);
matrix B(2,2);
A.MatrixArray[0][0]=1.0;
A.MatrixArray[0][1]=2.0;
A.MatrixArray[1][0]=3.0;
A.MatrixArray[1][1]=1.0;
B.MatrixArray[0][0]=3.0;
B.MatrixArray[0][1]=4.0;
B.MatrixArray[1][0]=1.0;
B.MatrixArray[1][1]=2.0;
display(A);
cout << endl;
display(B);
cout << endl;
matrix D(2,2);
D=add(A,B);
display(D);
cout << endl;
matrix E(2,2);
E=AB;
matrix F(2,2);
F=BA;
display(E);
cout << endl;
display(F);
system("PAUSE");
return EXIT_SUCCESS;
}
> So if I return an object instead of a reference, that should
> fix the problem? I tried that, but I still got junk.
Not exactly. You should return an object, but that is not the only problem with the code.
> Also, if I comment out the destructor in the matrix class, then everything works fine (see below).
Ouch! Because it does not delete the allocated memory, the data remains (and constitutes a severe memory leak!). However the class members and the MatrixArray pointer itself exist on the stack, and the memory they occupy may get later reused.
> Is this also a flawed solution, and why does it work in the first place?
Yes it is, and more by luck than judgement. It is relying on never releasing memory, and that the memory used by the local object itself is never reused after it is reclaimed.
> I don't quite understand what you're suggesting at the very end:
Which part? I said two things there.
For the first, see http://www.cs.caltech.edu/courses/cs11/material/cpp/donnie/cpp-ops.html (specifically the part that says Define your binary arithmetic operators using your compound assignment operators."). The same link describes overloading the assignment operator, but you will need a copy constructor too - see http://www.fredosaurus.com/notes-cpp/oop-condestructors/copyconstructors.html .
Note that a matrix class is a common topic in C++ and there are many existing implementations (usually template based allowing you to create matrices of different types).
Also get yourself a good text on C++ that covers robust class design and operator overloading.
Clifford