Copy Constructors, Destructors, Dynamic Arrays, & Swap

[Rex Imperator]

Hello, I'm trying to create a copy constructor, destructor, and a swap function for a class that creates a dynamic array like:

class A
{
public:
A(....)
{
dArray = new int [size];
}
A(const A& other)
:size(other.size)
{
    dArray = new int [size];
    for(int i = 0; i < size; i++)
        dArray[i] = other.dArray[i];
}
~A()
{
delete [] dArray;
}

int& operator=(const int& right)
{
if (this != &right)
{
A temp(right);
swap(temp);
}
}

void swap(A& other)
{
int tmp = size;
size = other.size;
other.size = tmp;
int* tmpPointer = dArray;
dArray = other.dArray;
other.dArray = tmpPointer;
}
private:
int* dArray;
int size;
};

Am I going in the right direction with this or am I making a bunch of dangling pointers and memory leaks? I'd appreciate some guidance if I am.

Thanks.

[laserlight]

If you intended to declare a copy assignment operator (and you probably did), then you are doing it wrong. It should be declared as:

A& operator=(const A& right)

and remember to return *this.

For swap, you can make use of std::swap.

Note that if you want to mimic a simplified version of std::vector, then you probably should store the capacity (number of elements for which space has been allocated), not just the size (number of elements that have been constructed/are in use).

[whiteflags]

A(....) {
  dArray = new int [size];
}

What is the interface for this constructor?

int& operator=(const int& right) {
  if (this != &right) {
    A temp(right); swap(temp);
  }
}

This copy assignment operator has a non-applicable interface. You need to read about the copy-swap idiom again, too. You're not doing it; swap has to exchange class data.

You also need to return *this because what if I do:

A foo, bar, quz;
...
foo = bar = quz;

Am I going in the right direction with this or am I making a bunch of dangling pointers and memory leaks? I'd appreciate some guidance if I am.

Almost certainly, but your major problem is the copy assignment operator.

[laserlight]

This copy assignment operator has a non-applicable interface.

That is, it is not a copy assignment operator. It is an assignment operator that allows assignment of integers to the object, with an unusual return type and missing return statement.

You need to read about the copy-swap idiom again, too. You're not doing it; swap has to exchange class data.

If that was intended to be copy assignment operator and was actually declared as such, then the implementation does correctly use the copy-swap idiom, except for the missing return statement.

[Rex Imperator]

Thanks for all the help everyone. I got it to work on my actual project,which is a bit different than the example I posted here). These are the basic changes I made. Hopefully, I didn't forget something/make a typo.

I didn't know there was a swap function, but that's good to know. I couldn't have used it on this project though. We had to make our own.

class A
{
public:
//on the interface: A(int maximum = MAXIMUM_SIZE);
A(int maximum) 
   :size(0)
{
size = 0;
maxSize = maximum;
dArray = new int [maxSize];
}

A(const A& other)
   :size(other.size),
{
dArray = new int [size];    
for(int i = 0; i < size; i++)
dArray[i] = other.dArray[i];
}

~A()
{
delete [] dArray;
}

A& operator=(const int& right)
{
if (this != &right)
{
A temp(right);
swap(temp);
}
return *this;
}

void swap(A& other)
{
int temp = size;
size = other.size;
other.size = temp;
temp = maxSize;
maxSize = other.maxSize;
other.maxSize = temp;
int* tmp = new int [maxSize];
tmp = dArray;
dArray = other.dArray;
other.dArray = tmp;

}
private:
int* dArray;int size;
int maxSize;
};

[laserlight]

I didn't know there was a swap function, but that's good to know. I couldn't have used it on this project though. We had to make our own.

You can use std::swap to implement your swap member function.

Your one parameter/default constructor probably should be declared explicit. Also, since you initialise size, there is no need to assign to it in the constructor body.

Your copy constructor should initialise both size and maxSize (which should be declared on different lines), and then perhaps it should create the dynamic array with maxSize rather than size.

Your copy assignment operator is still not what you intended: the parameter has a wrong type.

Your swap member function now has a memory leak; swapping pointers, which is what you did previously, was correct.

[Rex Imperator]

Thank you. =]. I fixed the swap function on my main project as well as the other suggested fixes.

[phantomotap]

O_o

Well, I don't think anyone else said it, so I will: your `swap' method should provide a non-member interface at the same namespace scope as the class to sort of mesh with `std::swap'. (It may be an `inline' function if you wish.)

void swap
(
    Test & fLHS
  , Test & fRHS
)
{
    fLHS.swap(fRHS);
}

Note: the `swap' method can be named however you wish, but the non-member function should be named exactly `swap'.

If you do that for your class, I can do this:

void MyFunction(/*???*/)
{
    using std::swap;
    Test s1;
    // ...
    Test s2;
    // ...
    swap(s1, s2);
    // ...
}

Which let's me call your specific `swap' instead of `std::swap'.

This concept will be very valuable down the line if you ever approach generics, but may even offer performance boosts with any such class depending on if and how you are using standard containers and algorithms.

Soma