Need help with assignment overloading

[Sagekilla]

I'm working on a program for working with matrices for a summer research project, and I realized that (I think, not 100% sure here) g++ wasn't able to generate the needed code to handle assignments whereas MSVC++ was.

My matrix class looks like this:


class matrix
{
public:
int sizex;
int sizey;
vector<vector<double>> values;

...
more stuff
...
};


I realize I need a assignment constructor so I can properly do things like:


matrix M = M * M;


Except I have no clue if I'm doing this right, as I'm still getting segfaults with this code:


matrix& matrix::operator= (const matrix& M)
{
if (this != &M)
{
values = M.values;
sizex = M.sizex;
sizey = M.sizey;
}
return *this;
}

I'm positive I'm doing this wrong. Can anyone point me in the right direction?

[KenD00]

Looking at your class snippet i don't see a need for an assignment operator, all members are primitives or STL classes of primitives, the default assignment operator will work just fine.

Btw. in your mentioned example not the assignment operator is used but the copy constructor, however this example doesn't make sense because you create M by assigning the product of itself. I would say this shouldn't even compile. Because i wasn't sure if it would i created a simple example myself and it did compile but the result was strange. So if you really compiled that code it is ok that it segfaults :D. But if you compiled something like this

matrix N;
matrix M = N * N;

i would check if the * operator is working correct because the assignment operator looks correct.

:rolleyes:

[Sagekilla]

Hm. Under MSVC++ it compiles fine (Odd huh?)

I did a break for both the assignment and multiplication operator, and the multiplication actually works perfectly fine. It gets passed to the assignment perfectly fine, but then segfaults on the return *this.

I'll try looking over and checking my multiplication operator. Here's the full code for what I'm doing:


matrix operator* (matrix& A, matrix& B)
{
int Ax = A.sizex, Ay = A.sizey;
int Bx = B.sizex, By = B.sizey;

if (Ax != By)
errMessage("Inner dimensions unequal for matrix multiplication");

matrix result(Ay, Bx);

for (int y = 0; y < Ay; y++)
for (int x = 0; x < Bx; x++)
for (int k = 0; k < Ax; k++)
result(y, x) += A(y, k) * B(k, x);

return result;
}



Update: Do I have to do something like this? I haven't had a chance to try compiling it yet (Running out to work soon) but I think it might fix the problem:


matrix& operator* (matrix& A, matrix& B)
{
int Ax = A.sizex, Ay = A.sizey;
int Bx = B.sizex, By = B.sizey;

if (Ax != By)
errMessage("Inner dimensions unequal for matrix multiplication");

matrix* result = new matrix(Ay, Bx);

for (int y = 0; y < Ay; y++)
for (int x = 0; x < Bx; x++)
for (int k = 0; k < Ax; k++)
(*result)(y, x) += A(y, k) * B(k, x);

return *result;
}

[Nic]

I'd post your full code to reproduce the segfault. There's nothing wrong with your assignment code.

-Nic

[Gavino]

Update: Do I have to do something like this?
...
matrix* result = new matrix(Ay, Bx);
...
return *result;
}
Yes, I think that's it. Previously you were returning a reference to local matrix that would go out of scope.

[Sagekilla]

I made a new copy and stripped down my code so I have a very minimalistic version of what I'm trying to do. Currently I'm doing this:


int size = 10;
long simulations = 1048576;

V results(simulations, 0);

for (long idx = 0; idx < simulations; idx++)
{
matrix A(size, 0.0, 1.0);
results[idx] = A.powerMethod();
A.~matrix();
}

sort(results.begin(), results.end());


powerMethod is defined as:


double matrix::powerMethod()
{
if (sizex != sizey)
errMessage("Must use square matrices for eigenvalue decomposition");

matrix A(*this);
matrix M(*this);

double eigen1 = -1, eigen2 = 0;

while (fabs(eigen1 - eigen2) > DIFF)
{
eigen1 = eigen2;

M = M * M;
M.normalize();

V EV = M.columnAsVector(0);
V A_EV = A * EV;
eigen2 = length(A_EV) / length(EV);
}

return eigen2;
}


V is just a typedef for vector<double> and columnAsVector and length should be self explanatory.

Right now I'm getting a memory leak using the second multiplication code (matrix& matrix::operator* (matrix& A, matrix& B).

Can anyone point out what I'm doing wrong here?

[KenD00]

Of course you are getting a huge memory leak, every multiplication creates a matrix with new that never gets deleted! Your first multiplication is just fine, the result is returned by value and not by reference.

And pls, can you do as Nic suggested, post something that we can run through a compiler and which segfaults when running. I looked through all your snippets and its still not enough to get something to compile, and i won't waste my time by completing your code, that makes no sense anyway because i don't see an error in your current code.

But i saw something very strange
A.~matrix();
Wtf, an explicit destructor call?!? This object even hasn't been created on the heap, and your class doesn't contain any members that create something on the heap! There is absolutly no need to call the destructor! I'm pretty sure that this is not allowed at all!

My current advise is throw away the assignment operator, remove the destructor call, to be sure make the parameters to the multiplication operator const to ensure that you don't modify the operands and post a complete source sample.

:rolleyes:

[cogman]

For the multiplication operator, you are really going to want to just return the matrix, and not return by reference. As has been pointed out, you are going to end up with a memory leak if you try and return it by reference.

Matrix destruction is done automatically when the matrix leaves scope, or the delete command is called. So there is never a need to explicitly call the destructor of an object.

Your multiplication method could be sped up by using something along the lines of Strassen's algorithm. Since your matricies have to be square anyways, it should work pretty well for you (you'll need a matrix addition and subtraction operator).