Passing class member function return value by refe

Question

I've been searching for a solution to this problem but can't seem to find one. I'm sure this general question has been asked before somewhere but hopefully you can help with my specific situation...

I have a class template someClass that contain the following (private) members:

int     size_x;
int     size_y;
int     size_total;
T *     grid;

someClass contains a constructor that looks like this:

someClass (const int x, const int y)
: size_x (x), size_y (y), size_total (x*y)
{
    grid = new T [size_total];
}

a copy constructor that looks like this:

someClass (const someClass & rhs)
{
    size_x = rhs.size_x;
    size_y = rhs.size_y;
    size_total = rhs.size_total;
    grid = new T [size_total];
    memcpy(grid, rhs.grid, size_total*sizeof(T));
}

a member function that looks like this:

T * retGrid (void) const
{
    return grid;
}

and an assignment operator that looks like this:

someClass & operator= (const someClass & rhs)
{
    if (this != &rhs)
    {
        size_x = rhs.size_x;
        size_y = rhs.size_y;
        size_total = rhs.size_total;
        grid = new T [size_total];
        memcpy(grid, rhs.grid, size_total*sizeof(T));
    }

    return *this;
}

I'm trying to pass the following two someClass objects

someClass<double> *I1 = new someClass<double>(10,10);

someClass<double> I2 = *I1;

to a function outside of the someClass class with the following prototype:

int someFunction(double *arr);

This call works fine:

int status;
status = someFunction(I1->retGrid()); // Properly working function call

but this does not

status = someFunction(&I2.retGrid()); // Compiler gives error that says "error: invalid lvalue in unary &"

And if I call someFunction like this:

status = someFunction(I2.retGrid()); // Compiler gives no error but function returns error value in status

the code compiles but I get a run-time error (a bad status value from another function call within someFunction).

How can I properly pass I2 to someFunction?

Many thanks...

Answer 1

You are trying to take address of the temporary object (in this case, a pointer but this doesn't really matter) returned by retGrid. Hence you cannot use the & approach.

Without the ampersand, you would pass the internal array from I2 to someFunction. If that's not fine for you (i.e. since you get some kind of runtime error), consider making a copy of this array and passing it to someFunction instead.

Answer 2

First question: why aren't use using std::vector, instead of trying to manage the memory yourself. You don't show the destructor; I presume you free the memory there. But there are still problems:

• In the copy constructor, you use memcpy. That's not a problem when you instantiate over double, but it could be a problem for other types. You should be using std::copy.

• If you were using std::vector, and retGrid still needed to return a T*, it would be return &grid[0];.

• The assignment operator is broken. It leaks any previous memory, and if the new fails, it leaves the object in an inconsistent state. (Having to check for self-assignment is usually a hint that something is wrong.) A correct assignment operator will do all operations which might fail before changing anything in the object. You might search for information about the swap idiom, but something like the following would also work:

.

SomeClass&
SomeClass<T>::operator=( SomeClass const& other )
{
    T* newGrid = new T[other.size_total];
    std::copy( other.grid, other.grid + other.size_total, newGrid );
    delete [] grid;
    size_x = other.size_x;
    size_y = other.size_y;
    size_total = other.size_total;
    grid = newGrid;
    return *this;
}

You might want to optimize this if the size_total are equal (or size_total <= other.size_total).

And of course, if you use std::vector, the compiler generated assignment operator and copy constructor are sufficient; you don't have to write anything.

• Is there any reason why you use a pointer for I1? (Or is this just an artifact of a larger context from which you extracted the code?)

• Concerning someFunction( &I2.retGrid() );, someClass::retGrid() returns a pointer. Regardless of whether you call the function through a pointer or an object. Taking the address results in a T**.

• Concerning the last call, there is nothing that would cause a problem here in the code you've shown us. As long as I2 is in scope and hasn't deleted it's object, there should be no problem. This is the correct way of calling the function on an object. The problem in your code is elsewhere.

Answer 3

I'm not sure of the runtime error that you are getting but status = someFunction(&I2.retGrid()); is passing pointer to a temporary object.

The runtime error could be because of missing copy constructor that is invoked in someClass<double> I2 = *I1;

Answer 4

When you have pointers in your class and you have to allocate memory for each object, you need to define a copy constructor and an assignment operator. You only added the later. This initialization

someClass<double> I2 = *I1;

is actually performed with a copy constructor, not with the assignment operator. It is identical to

someClass<double> I2(*I1);

However, it is wrong, because you only allocate memory for grid. But if grid was already allocated (from a previous assignment) you'd leak memory. So it should look like this:

someClass & operator= (const someClass & rhs)
{
    if (this != &rhs)
    {
        size_x = rhs.size_x;
        size_y = rhs.size_y;
        size_total = rhs.size_total;
        delete [] grid;
        grid = new T [size_total];
        memcpy(grid, rhs.grid, size_total*sizeof(T));
    }

    return *this;
}

Answer 5

Thank you to everyone for the extremely useful comments, especially those by AAT, David Rodriguez - dribeas, James Kanze, and Marius Bancila.

It turns out the problem was improper interface with a third-party function inside someFunction. I found and fixed the error after some sleep and now the call:

status = someFunction(I2.retGrid()); // Compiler gives no error but function returns error value in status

works correctly.

But this discussion brought other very important related issues to light, namely the memory management issues related to my copy constructor and assignment operator and suggested use of vectors. I believe this thread has a lot of value on those merits.