C++ Template Copy Constructor Deep Copy

Go To StackoverFlow.com

2

I'm writing an generic Array class and overloading operators for convenience. I've gotten my Array<> object to hold other Array<> objects, but I'm having trouble overriding the * operator. I need to copy the left object, so my operator* code is this:

Array<T>& operator*(const double scalar) {
    return Array<T>(*this) *= scalar;
}

(operator*= has been overloaded and works).

I overrode the copy constructor as below:

UPDATE: New copy constructor:

Array<T> (const Array<T>& copyfrom) {
    size_=copyfrom.size();
    data=new T[size_];
    copy(&copyfrom.data[0], &copyfrom.data[size_], data);
}

My thought is that if the array were not generic but always filled with a primitive, this code would work. But I think there's something going on here because I'm using a template that's causing behavior I don't expect. The data array in the new "deep copied" Array acts like it's just a shallow pointer copy of the "copyfrom" data array.

How do I made this copy constructor work for both primitives and objects, using templates? Or, better yet, is there a way to overload operator* without worrying about copy constructors? Thanks.

EDIT: Here is the code for operator*=. I still think my problem lies in my use of the copy constructor, however.

Array<T>& operator*=(const double scalar) {
    for (int i=0; i<size_; i++)
        data[i]*=scalar;
    return *this;
}

EDIT: I realized that I was getting problems because I was ignoring the size of my inner arrays in my array of arrays. Things are more reliable now. Everyone's been really helpful, and I think I'm on the way to making this work. My operator= (which I hadn't overloaded; good catch, Michael) is now as below. It acts as expected, but I'm starting to get malloc errors in my program and I'm exploring why. Is there anything wrong with the memory management here?

Array<T>& operator=(const Array<T>& a) {
    if (this==&a)
        return *this;
    delete [] data;

    size_=a.size();
    data=new T(size_);
    copy(&a.data[0], &a.data[a.size()], data);

    return *this;
}

EDIT: I fixed the malloc errors! My methods are all working as intended now. The memory problems were coming because I had this method header:

template <typename T>
static Array<T> matrixSolve (Array<Array<T> > m);

I was taking an array of arrays by value. All sorts of problems. Took the array by reference and everything worked out. Thanks for the help, everyone!

2012-04-05 02:49
by Enigmoid
@MichaelAnderson The code for operator* shouldn't return this. The call of Array<T>(*this) calls the copy constructor, which I've confirmed. Also, I don't get a compile error when I use operator*= if I make operator* const, so I must not be mutating this - Enigmoid 2012-04-05 11:01
You are correct - I'll delete those comments - Michael Anderson 2012-04-05 12:21
Can you show us Array<T>::operator= ? When you're using data[i]=copyfrom.data[i] you are invoking the operator= for the data. If thats not defined you'll get the default one. So If you nest Arrays and Array doesn't have = then bad things will happen - Michael Anderson 2012-04-05 12:25
Is there a reason why you're not using std::vector or std::array for the data member? You'd then probably have the default copy constructor and assignment operators do the right thing - Michael Anderson 2012-04-05 12:27
@MichaelAnderson I think you hit it; I'm nesting arrays and invoking operator= on an Array<T> object. That's probably a problem. I'm going to overload operator= and I'll get back to you - Enigmoid 2012-04-06 02:58
@MichaelAnderson Thanks for all your help. I wish I could accept comments as answers; if you post an answer I'll accept it - Enigmoid 2012-04-06 19:22


3

You'll want your operator* member function to look something like this:

Array<T> operator*(const double scalar) const {
    Array<T> result(*this);
    result *= scalar;
    return result;
}

If you return an Array<T> &, then you will be returning a reference to a temporary which will be destroyed, causing undefined behavior.

2012-04-05 02:56
by Vaughn Cato
You probably want to make that operator* const too - Michael Anderson 2012-04-05 03:02
@MichaelAnderson: true -- change - Vaughn Cato 2012-04-05 03:52
There is nothing bad about pulling together those three lines to one, as in the OP's original code; in fact I consider it more readable because you don't introduce the noise of declaring a temporary variable name and it's immediately visible that you're essentially doing "*this *= scalar with a backup".   It was just the return-by-reference and the missing const that was wrong - leftaroundabout 2012-04-05 11:14
@leftaroundabout: I don't think that many people would agree. Using (lhs *= rhs) as an rvalue equal to lhs isn't that readable - MSalters 2012-04-05 11:29
Ads