Thread: trouble returning an object by reference

Hello all,
Could anyone tell me why I am losing my object? and possibly how I could correct this problem?

I am having alot of trouble with this function. I would like to return an object by reference from a function, but somewhere between my "return" statement and assigning the result the reference to my returned object gets lost. I don't know why but the destructor is fired by the time I get back inside my assignment operator.

I have tried to include the least amount of code needed to show my problem. Please forgive me for its lengthiness.

Some how after the returning from the operator* and after just entering the assignment (=) operator my returned matrix object is lost. When I go through debugging, the object is still good but is lost before the "if (right.get_length()!=get_length)" statement. I identified this by watching the object.aptr value change from what it was before the return statement. It appears to still be correct when I get to the first line of the assignment operator definition but not after that.

Thanks for any comments.
-fisher

Code:

#include "Matrix.h"

int main(){
    matrix<int> MAT(3),MAT2(3);

    for (int j=1; j<=3; j++){
        for (int i=1; i<=3; i++){
            MAT(i,j)=1;
        }
    }

    MAT2=MAT*MAT; //at the moment, this is the troublesome line <---------------------------
   
    return 0;
}

Code:

//Matrix.h
#ifndef MATRIX_H
#define MATRIX_H
#include <iostream>
#include <cstdarg> //for using variable argument functions

template <class T>
class matrix
{
protected:
  T *aptr;
  int *size; //adjustable array of size of each matrix dimension
  int dim; //number of dimensions of matrix
  int array_length; //total number of elements in matrix
    // array_length=size[0]*size[1]*...*size[dim-2]*size[dim-1]
  char type;
public:
  // CONSTRUCTOR PROTOTYPES
  matrix();//default constructor
  matrix(int);// two-dimensional square matrix constructor
  matrix(int, int, ...);//an any dimensional matrix  
  ~matrix(){cout<<"matrix distroyed"<<endl;}

  //*** --- operators ----***
  T &operator()(int, ...);//reference operator: matrix(i,j,...)=matrix[i-1][j-1]...
  const matrix &operator= (const matrix &);//assignment operator
  //multiplication
  const matrix &operator* (matrix &);
};

template <class T>//an any dimensional array
matrix<T>::matrix(int d, int n, ...){
  int i;
  va_list list;
  if(d>0){
    dim=d;
    size= new int[dim];
    size[0]=n;
    array_length=n;
    va_start(list, n);
    for(i=1;i<d;i++){
      size[i]=va_arg(list,int);
      array_length *= size[i];
    }
    aptr= new T[array_length];
    for (i=0;i<array_length;i++)
      aptr[i]=0;
  }
  va_end(list);
}

template <class T>//reference operator
T &matrix<T>::operator()(int row, ...){
  int i, prod=0, element=0;
  va_list list;
  va_start(list, row);
  prod=size[0];
  element= (va_arg(list,int)-1)+size[1]*(row-1);
  for(i=2;i<dim;i++){
    prod*= size[i-1];
    element+= prod*(va_arg(list,int)-1);
  }
  va_end(list);
  return aptr[element];
}

template <class T> //assignment operator
const matrix<T> &matrix<T>::operator= (const matrix<T> &right){<------object lost between here
  if(&right != this){ //check for self-assignment <-------------------------------------and here
    //reallocate memory if different amount of space needed
    if( array_length!=right.array_length ){
      array_length=right.array_length;
      dim=right.dim;
      delete [] aptr;
      delete [] size;
      size= new int[dim];
      for(int i=0; i<dim ; i++) size[i]=right.size[i];
      aptr= new T[array_length];
    }
    else{//change size of dimensions if necessary
      if(dim!=right.dim){ 
        dim=right.dim;
        delete [] size;
        size= new int[dim];
      }
      for(int i=0; i<dim ; i++){
        if (size[i]!=right.size[i])
          size[i]=right.size[i];
      }
    }
  }
  for(int i=0; i<array_length; i++) 
    aptr[i]=right.aptr[i]; //copy matrix elements
  return *this;
}

template<class T>//assumes 2D matrixes
const matrix<T> &matrix<T>::operator* (matrix<T> &right){
  T sum;
  if((*this).size[1]==right.size[0]){
    matrix<T> new_m(2,(*this).size[0],right.size[1]);
    //do dot product of rows and columns
    // in tensor form Cik=Aij*Bjk (sum over j-index)
    for(int i=1;i<=(*this).size[0];i++){
      for(int k=1;k<=right.size[1];k++){
        sum=0;
        for (int j=1; j<=(*this).size[1]; j++){
          sum+=(*this)(i,j)*right(j,k);
        }
        new_m(i,k)=sum;
      }
    }
    return new_m;//this might not be passing back correctly <-----------------------------------
  }
}
#endif

Your code produces a number of errors for me.

Code:

Matrix.h:101: warning: reference to local variable `new_m' returned

Bad idea.

Originally Posted by fisheromen1031

Hello all,
I am having alot of trouble with this function. I would like to return an object by reference from a function, but somewhere between my "return" statement and assigning the result the reference to my returned object gets lost. I don't know why but the destructor is fired by the time I get back inside my assignment operator.

You might like to return a reference to new_m, but you can't do that because you are not allowed to return a reference to a local variable. That variable ceases to exist when the function has returned and you have a dangling reference.
So the solution is to stop trying to do things that are forbidden.
An even better solution would be to use expresion templates which help eliminate temporaries. For that I'd recommend dropping your implementation and using the CML

Originally Posted by fisheromen1031

I am doing that on purpose. It seemed like the best way for me to accomplish things.

If you had a vehicle that was just above the weight limit for a bridge would you take the wheels off to cross it? That's about as bad as what you've done.

Damn I'm getting my mileage out of this analogy today...

If the "best way" is to do something stupid, then I shudder to think of the rather obtuse ways you probably considered.

A local variable, in the form of an object, is destroyed upon the end of the function (more or less).

Let me give you an example where this "technique" provides for unwanted behavior:

Code:

#include <iostream>

class abc
{
	int x;

public:
	abc()
	{
		std::cout << "abc::abc()" << std::endl;
	}
	~abc()
	{
		std::cout << "abc::~abc(); integer = " << x << std::endl;
	}
	void set(int i)
	{
		std::cout << "abc::set(); value = " << i << std::endl;
		x = i;
	}
	void print()
	{
		std::cout << "abc::print()" << std::endl;
		std::cout << "x = " << x << std::endl;
	}
};

abc &create(int);

abc &create(int i)
{
	abc r;
	r.set(i);
	return r;
}

int main()
{
	std::cout << "Before the horrible create()..." << std::endl;
	
	abc &z = create(10);

	std::cout << "After the horrible create()...." << std::endl;

	z.set(6);

	std::cout << "Altered value." << std::endl;
	
	z.print();
	
	std::cout << "Done." << std::endl;
	
	return 0;
}

Example output on a system of mine:

Code:

Before the horrible create()...
abc::abc()
abc::set(); value = 10
abc::~abc(); integer = 10
After the horrible create()....
abc::set(); value = 6
Altered value.
abc::print()
x = 4201217
Done.

As you can see, returning a local variable totally screws up the life of the object... because the object actually dies, but you're using it anyway.

Now you are returning a value, not a reference.

Because your new example doesn't pass a REFERENCE to the object, but a copy of the object, which means that the new_m that you have as a local variable doesn't get used after it has been "removed".

--
Mats

Well, you are saving some overhead, but unfortunately, by using data that is about to be re-used for other purposes - a bit like filling your homebrewed beer into the bottles that are in the recycling bin, leaving them in there to settle for a few days, and then act surprised when you can't find your beer-bottles as the recycling men have collected them... [Keeping your beer in the recycling bin also saves time when you have finished the beer...]

--
Mats

I thought I was being clever and potentially saving myself some overhead for when very large matrixes might need to be passed back and forth.

You do have a point there, though. In some cases you might want to save the overhead of copying and operate on the current matrix itself. As such, I suggest that you implement operator*= for scalar multiplication:

Code:

template<typename T>
matrix<T>& matrix<T>::operator*=(const T& n) {
  for (int i = 0; i < array_length; ++i) {
    aptr[i] *= n;
  }
  return *this;
}

Then you can implement operator* for matrix multiplication in terms of the assignment operator version:

Code:

template<typename T>
matrix<T> matrix<T>::operator*(const T& n) {
  return matrix<T>(*this) *= n;
}

So you get the best of both worlds: if you want to change the current matrix, you can do so, and if you want to generate a new matrix, you can also do so.