3

I was trying to write a matrix class which would be able to find inverse,adjoint,etc. of a square matrix of any order. The constructor initializes an identity matrix of order n(passed to it). 

class Matrix
{
int** elements;
int order;

public:
Matrix& operator=(const Matrix& second_inp)
{
    if(this->order!=second_inp.order)
        cout<<"The matrix cannot be assigned!!!\n"<<this->order<<"\n"<<second_inp.order;

    else
    {
        for(int i=0;i<this->order;i++)
            for(int j=0;j<this->order;j++)
                this->elements[i][j] = second_inp.elements[i][j];

    }

    return *this;
}

Matrix operator*(const Matrix& a)const
{
    Matrix c(a.order);

    for(int i=0;i<c.order;i++)                      
        for(int j=0;j<c.order;j++)
            c.elements[i][j]=0;

    if (this->order!=a.order)
    {
        cout<<"The 2 Matrices cannot be multiplied!!!\n";
        return Matrix();
    }

    else
    {
        for(int i=0;i<a.order;i++)
            for(int j=0;j<a.order;j++)
                for(int k=0;k<a.order;k++)
                    c.elements[i][j] += (this->elements[i][k])*(a.elements[k][j]);

        return c;
    }
}
};

~Matrix()
{
    for(int i=0;i<this->order;i++)
        delete[] *(elements+i);
    delete[] elements;
    elements=nullptr;
}

If i were to run the following code using this class:

Matrix exp1(2),exp2(2),exp3(2);
exp1.get_matrix();
exp3=exp1*exp2;
exp3.show_matrix();

I get a run-time error, while debugging i found out that, after the multiplication(exp1*exp2) the =operator was not able to access the data if the result of the *operator.

But if i were to use a manual destructor like this one at the end of the main() to free all allocated memory, the program works fine.

void destroctor()
{
  for(int i=0;i<order;i++)
    delete[] *(elements+i);
  delete[] elements;
}

how can i edit the destructor or the operator overloads to correct this problem?

The constructor i used:

Matrix(int inp_order):order(inp_order)
{
    elements=new int*[order];

    for(int i=0;i<order;i++)
        *(elements+i)=new int[order];

    for(int i=0;i<order;i++)
        for(int j=0;j<order;j++)
        {
            if (i==j)
                *(*(elements+j)+i)=1;
            else
                *(*(elements+j)+i)=0;
        }
}
Likhit
  • 799
  • 2
  • 6
  • 18
  • Is the allocation of `elements` missing in your code too (and not just in the question)? – Eran Sep 18 '11 at 10:58
  • No the allocation is not missing in the actual code.I added the constructor to the question. – Likhit Sep 18 '11 at 11:04
  • 1
    Yes i tried returning by reference too but it just returns a bunch of warnings saying that i'm referencing memory which has been deleted already. – Likhit Sep 18 '11 at 11:09
  • @Likhit: If you want a perfect program, you shouldn't leave any warning :) – Tamer Shlash Sep 18 '11 at 11:17
  • Show us your copy constructor (if you don't have one, then that's your bug). – JoeG Sep 18 '11 at 11:21
  • Slightly OT: unless it is an exercise that you do for its own sake and just need matrices, go for [eigen](http://eigen.tuxfamily.org). – eudoxos Sep 18 '11 at 11:44

3 Answers3

3

It is hard to tell what is going wrong, since you have not posted your constructors.

In the exp3=exp1*exp2; a lot of things happen:

First a new matrix c is constructed in the operator* function. Then the return c; statement calls the copy constructor and then the destructor. After that operator= is called and after that the destructor for the temporary matrix again.

I think what happens is that you are using the default copy constructor which does not make a deep copy. That way the destructor being called at the time of return c deletes the data that still shared between the matrices.

plaisthos
  • 6,255
  • 6
  • 35
  • 63
  • It probably lacks a copy constructor indeed. – Shautieh Sep 18 '11 at 11:10
  • Yes i have used no copy constructor. I assumed that the 'operator=' works the same way as a copy constructor and therefore will be called directly.. Thanks. – Likhit Sep 18 '11 at 11:17
  • @Likhit: If you don't supply a copy constructor, the compiler will provide one for you. In this case, it simply copies the pointer value `elements`. This is a "shallow copy" when you need to always make a "deep copy" in the copy constructor and assignment operator overloads. – Daniel Trebbien Sep 18 '11 at 11:24
0

I get a run-time error, while debugging i found out that, after the multiplication(exp1*exp2) the =operator was not able to access the data if the result of the *operator.

You didn't show us your constructor, so there is no way to tell why you are getting this errors.

I suspect that the cause is that you aren't allocating the memory needed to contain your matrix. You declared it as an int**, so you need to allocate an array of int* pointers, and for each of those you need to allocate an array of int.

Edit
While I was typing this you posted code for your constructor.

You are not returning a value from your overload of operator*, and you don't have a copy constructor (rule of three).

Do you have compiler warnings enabled? Any compiler worth its salt would have complained about the missing return statement in the operator overload.

David Hammen
  • 32,454
  • 9
  • 60
  • 108
  • I'm sorry but i am returning the temporary matrix(c) in my 'operator*' function, it is present in the 'else' statement. – Likhit Sep 18 '11 at 11:15
  • I didn't see that return. In a way that is even worse. C++ has exceptions. Use them. Or call `exit()`. Don't return a bogus value when you have a serious error like this. Even better is to make this problem a compile-time error, but that requires the use of templates. – David Hammen Sep 18 '11 at 12:05
0

You have not defined a copy constructor, so the compiler will generate one for you. This constructor will be called in order to copy the return value of operator*(const & Matrix a) into the result.

As the generated copy constructor only performs a shallow memberwise copy, it will not allocate a new array of elements, hence the error.

Andrew Marshall
  • 1,469
  • 13
  • 14