adding two matrix

[vaibhav]

Following is code for adding two matrix.For a 2 by 2 matrix it is giving first two value as garbage value and next two as same.
The problem is with overloaded+operator function(shaded ).Before returning if I cout the added matrix it prints fine but after returning it is not printing correctly.Even it is not returning *this correctly.

#include<iostream.h>
#include<conio.h>

class matrix
{
private:
  int *ptr,n;

public:
  matrix(int num):n(num)
  {                         //constructor
  ptr=new int[n*n];
  }

  void getmatrix();
  void putmatrix();
  matrix operator+(matrix &);

  ~matrix()
  {                 //destructor
  delete[]ptr;
  }
};

void matrix:: getmatrix()
{
cout<<"Enter the matrix";
for(int i=0;i<=n-1;i++)
{
  for(int j=0;j<=n-1;j++)
    cin>>ptr[i*n+j];
}
}

void matrix::putmatrix()
{
for(int i=0;i<=n-1;i++)
{
  for(int j=0;j<=n-1;j++)
      cout<<ptr[i*n+j]<<"\t";
cout<<endl;
}
}


matrix matrix::operator+(matrix &second)
{
matrix temp(n);
for(int i=0;i<=n-1;i++)
{
  for(int j=0;j<=n-1;j++)
  temp.ptr[i*n+j]=ptr[i*n+j]+second.ptr[i*n+j];
}
//couting temp here works fine
return(temp);//Even it does not return(*this) correctly
}
int main()
{
int m,n;
cout<<"Enter no of rows and columns";
cin>>m>>n;

if(m!=n)
  cout<<"Addition not possible";

else
{
  matrix m1(n),m2(n),m3(n);
  m1.getmatrix();
  m1.putmatrix();
  m2.getmatrix();
  m2.putmatrix();
  m3=m1+m2;
  m3.putmatrix();
}
getch();
return(0);
}

[JaWiB]

The problem here is that the object you return contains variables that have been dynamically allocated. After the function returns, its destructor is called and the memory is freed.

To be honest, I'm not sure what the best solution is. You could make your function return a reference to a dynamically allocated matrix object, ie:

matrix& matrix::operator+(matrix &second)
{
matrix* temp = new matrix(n);
//...
return(*temp);
}

You should also note, however, that the compiler has defined its own assignment operator for your code, and that (AFAIK) it will be a shallow copy. In this case, you would have a memory leak because you allocate memory for m3, then on this line:

m3=m1+m2;

The pointer is reassigned and you lose the address to the memory you allocated. You should probably define a default constructor that does not allocate any memory, as well as your own assignment operator, which, I suppose, will have to perform a shallow copy. You can then check to see if ptr is valid in the assignment operator, and if it is delete it.

~matrix()
{
delete ptr;
ptr = 0; //add this line
}
matrix& operator=(const matrix& rhs)
{
//...
if (ptr)
  delete ptr;
//...
}

The problem with this solution is that by performing a shallow copy in the assignment operator, you have to be careful not to assign a matrix that will be destroyed, ie:

matrix a(5);
matrix b;
b = a;
a.~matrix();
//b's data is invalid

[Thantos]

consider using a vector and initalization to make deep copies. Also think about using const

[vaibhav]

i have not allocated temp (inside greened code) dynamically. I don't this there is any problem in returning temp.Even return(*this) is giving returning this correctly.ie.for a 2*2 matirix first row's value's are garbage.I think return is not working correctly or there is some problem with compiler's overloaded = operator( as i have not supplied my own).I haven't studied vectors yet. So I have to make this through arrays only.

[7stud]

i have not allocated temp (inside greened code) dynamically

...but in the green, you've dynamically allocated a member of temp when temp calls the constructor.

You pass arguments to functions "by value" or "by reference", but you also return values from functions "by value" or "by reference". You are returning temp "by value" which means temp is copied and the copy of temp is returned. The copy of temp is made with the default copy constructor, which is automatically supplied by the compiler. The default copy constructor makes a "shallow" copy: it just blindly copies member for member. So, the copy of temp has a member that is assigned the address stored in temp's member, i.e. they both point to the same memory.

Now, what happens to the local variables in a function when the function ends? They get destroyed. temp is a local variable in your function; and when the function ends, your temp object calls its destructor which releases its member's dynamically allocated memory. The problem is the copy of temp that is returned from the function has a member that points to the same memory that temp's destructor released. Therefore, the pointer is invalid.

[vaibhav]

Thanks your explanation worked fine.But how can we explain return(*this) in place of temp(in greened code) not returning correct values as no destructor for this was called.

[vaibhav]

I have understood myself that we passed m1 by value and new variables were created which were destroyed by destructor after returning.Thus *this was also not able to send correct output.
Above program works fine if I remove destructor.

What neccessary changes should I make 
so that above program runs correctly retaining destructor.

[grumpy]

The problem is that you have not provided a copy constructor that works correctly. The effect of that is, when you create a copy of a matrix, that you have two matrices referrring to the same dynamically allocated data. Your problem comes in in your code (eg return *this; within your operator+()) because that statement invokes the copy constructor implicitly, and either the original or the copy is destroyed as the function returns. Any attempt to access the data in the surviving object yields undefined behaviour.

Try something like this;

matrix::matrix(const matrix &m) : ptr(new int[m.n*m.n]), n(m.n)
{
       for (int i = 0; i < n*n; ++i)
           ptr[i] = m.ptr[i];
}

In practice, if you need a hand-rolled copy constructor, you also need a hand-rolled assignment operator. One way of doing this (by using the hand-rolled copy constructor) is;

matrix &matrix::operator=(const matrix &m)
{
      // create a copy of m
      //    then swap members in the copy with the corresponding members in *this

      matrix copy(m);
      
      int *temp = copy.ptr;
      copy.ptr = ptr;
      ptr = temp;

     int itemp = copy.n;
     copy.n = n;
     n = itemp;

     return *this;
}