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...
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.
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;
}
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.
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;
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.
