Frettsie
Frettsie

Reputation: 51

Copy constructor not deep copying, error returning with array empty

I am doing a project for school and it requires a copy constructor, destructor, etc. When I use the copy constructor an error is thrown saying the array is empty, I assume the copy constructor is not working.

When I delete the copy constructor the program works, meaning the issue is probably happening within that function.

template<class T>
class DynArray {
public:

DynArray<T>(){
 ptr = new T[Capacity];
 this->Capacity = 2;
 this->Size = 0;  
}

DynArray<T>(T n) {
 ptr = new T[Capacity];
 this->Capacity = n;
 this->Size = 0;
}

DynArray<T>(const DynArray& orig) {
 cout << "Copy" << endl;
 ptr = new T[Size];
 *ptr = *(orig.ptr);
}

DynArray<T>& operator=(const DynArray<T>& orig) {
 if(this != &orig) {
 delete[] ptr;
 ptr = new T[Size];
 *ptr = *(orig.ptr);
}
return *this;  
}

void push_back(const T& n) {
 if (Size >= Capacity) {
 adjust(Capacity * 2);
 }
 ptr[Size] = n;
 Size++;
} 

void adjust(T a) {
 cout << "grow" << endl;
 T* arr = new T[a];
 for (int i = 0; i < Capacity; ++i) {
 arr[i] = ptr[i];
}
Capacity = a;
ptr = arr;
}

T& back() {
 if(Size == 0) {
 throw runtime_error("Array is empty");  
}
return ptr[Size - 1];
}

T& front() {
 if(Size == 0) {
 throw runtime_error("Array is empty");  
}
return ptr[0];
}

private:
 T* ptr = nullptr;
 int Capacity;
 int Size;

main:

#include <iostream>
#include "dynarray.h"
using namespace std;

int main( )
{
const char START = 'A';
const int MAX = 12;

// create a vector of chars
DynArray<char> vectD;

// push some values into the vector
for (int i = 0; i < MAX; i++)
{
    vectD.push_back(START + i);
}

// remove the last element
vectD.pop_back();

// add another value
vectD.push_back('Z');

// test memory management
DynArray<char> vectD2 = vectD;
// display the contents
cout << "\n[";
for (int i = 0; i < vectD2.size() - 1; i++)
{
    cout << vectD2.at(i) << ", ";
}

cout << "..., " << vectD2.back() << "]\n";

DynArray<char> vectD3;
vectD3 = vectD2;
cout << "\n[";
for (int i = 0; i < vectD3.size() - 1; i++)
{
    cout << vectD3.at(i) << ", ";
}
cout << "..., " << vectD3.back() << "]\n";

vectD3.front() = '{';
vectD3.back() = '}';
cout << vectD3.front();
for (int i = 1; i < vectD3.size() - 2; i++)
{
    cout << vectD3.at(i) << ", ";
}
cout << vectD3.at(vectD3.size()-2) << vectD3.back() << endl;
}

Later in my code I have it set to throw a runtime_error if Size == 0, it does throw the error meaning that the array is empty. Is the copy constructor copying correctly? Main cannot be changed, it is a given from the prof.

UPDATE: I changed the copy constructor to copy all of the elements of the array, but the runtime_error is still returning saying the array is empty.

template<class T>
class DynArray {
public:

DynArray<T>(){
 ptr = new T[Capacity];
 this->Capacity = 2;
 this->Size = 0;  
}

DynArray<T>(T n) {
 ptr = new T[Capacity];
 this->Capacity = n;
 this->Size = 0;
}

DynArray<T>(const DynArray& orig) {
 cout << "Copy" << endl;
 ptr = new T[Size];
 for (int i = 0; i < Size; i++) {
 ptr[i] = orig.ptr[i];
}
}

DynArray<T>& operator=(const DynArray<T>& orig) {
 if(this != &orig) {
 delete[] ptr;
 ptr = new T[Size];
 for (int i = 0; i < Size; i++) {
 ptr[i] = orig.ptr[i];
}
}
return *this;  
}

void push_back(const T& n) {
 if (Size >= Capacity) {
 adjust(Capacity * 2);
 }
 ptr[Size] = n;
 Size++;
} 

void adjust(T a) {
 cout << "grow" << endl;
 T* arr = new T[a];
 for (int i = 0; i < Capacity; ++i) {
 arr[i] = ptr[i];
}
Capacity = a;
ptr = arr;
}

T& back() {
 if(Size == 0) {
 throw runtime_error("Array is empty");  
}
return ptr[Size - 1];
}

T& front() {
 if(Size == 0) {
 throw runtime_error("Array is empty");  
}
return ptr[0];
}

private:
 T* ptr = nullptr;
 int Capacity;
 int Size;

Upvotes: 1

Views: 347

Answers (2)

Paul92
Paul92

Reputation: 9062

Your copy constructor,

DynArray<T>(const DynArray& orig) {
 cout << "Copy" << endl;
 ptr = new T[Size];
 *ptr = *(orig.ptr);
}

does not perform a deep copy.

A pointer, such as your ptr, is just an index to a resource (some memory block). If you copy it, the memory block is still the same and unique, you just have copied its address.

In order to perform a deep copy, you need to allocate a new block of memory (which you do), and then copy it. To copy a block of memory means to copy each of its elements. Here, you copy only the first. Therefore,

DynArray<T>(const DynArray& orig) {
 cout << "Copy" << endl;
 ptr = new T[Size];
 for (size_t i = 0; i < Size, i++)
     ptr[i] = orig.ptr[i];
}

Note that a[i] is the same as *(a+i), which means "the thing at the i'th position in the memory block".

Even better, you can use std::copy to hide the details of the deep copy operation, thus replacing the for loop:

std::copy(std::begin(orig.ptr), std::end(orig.ptr), std::begin(ptr));

in C++11, or:

std::copy(orig.ptr, orig.ptr + Size, ptr);

Upvotes: 0

Remy Lebeau
Remy Lebeau

Reputation: 596332

There are quite a few mistakes in your class.

The default constructor uses Capacity to allocate the array, before Capacity has been initialized.

Your reserving constructor is declared wrong. Its input parameter should be an int instead of a T. And it suffers from the same Capacity error as your default constructor.

Your copy constructor suffers from the same initialization error as well, only with Size instead. And it does not perform a deep copy at all. It is merely copying the ptr pointer from one class instance to another without regard to what is being pointed to. Same with your copy assignment operator.

Your adjust() method is also declared wrong, and it is leaking memory.

With that said, try something more like this instead:

#include <algorithm>
#include <utility>

template<class T>
class DynArray
{
public:

  DynArray(int n = 2)
    : ptr(new T[n]), Capacity(n), Size(0)
  {
  }

  DynArray(const DynArray& orig)
    : DynArray(orig.Size)
  {
    std::cout << "Copy" << std::endl;
    std::copy(orig.ptr, orig.ptr + orig.Size, ptr);
    Size = orig.Size;
  }

  DynArray(DynArray&& orig)
    : ptr(nullptr), Size(0), Capacity(0)
  {
    orig.swap(*this);
  }

  ~DynArray()
  {
    delete[] ptr;
  }

  DynArray& operator=(const DynArray& orig)
  {
    if (this != &orig) {
      DynArray(orig).swap(*this);
    }
    return *this;  
  }

  DynArray& operator=(DynArray&& orig)
  {
    DynArray(std::move(orig)).swap(*this);
    return *this;  
  }

  void swap(DynArray &other)
  {
    std::swap(other.ptr, ptr);
    std::swap(other.Capacity, Capacity);
    std::swap(other.Size, Size);
  }

  void push_back(const T& n)
  {
    if (Size >= Capacity) {
      grow();
    }
    ptr[Size] = n;
    ++Size;
  } 

  void pop_back()
  {
    if (Size <= 0) {
      throw std::runtime_error("Array is empty");  
    }
    ptr[Size - 1] = T();
    --Size;
  }

  T& front()
  {
    if (Size <= 0) {
      throw std::runtime_error("Array is empty");  
    }
    return ptr[0];
  }

  T& back()
  {
    if (Size <= 0) {
     throw std::runtime_error("Array is empty");  
    }
    return ptr[Size - 1];
  }

  T& at(int i)
  {
    if ((i < 0) || (i >= Size)) {
      throw std::out_of_range("Index out of range");
    }
    return ptr[i];
  }

  T& operator[](int i)
  {
    return ptr[i];
  }

  int size() const {
    return Size;
  }

  int capacity() const {
    return Capacity;
  }

private:
  T* ptr = nullptr;
  int Capacity = 0;
  int Size = 0;

  void grow()
  {
    std::cout << "grow" << std::endl;
    DynArray newArr(Capacity * 2);
    std::copy(ptr, ptr + Size, newArr.ptr);
    newArr.Size = Size;
    newArr.swap(*this);
  }
};

Then, you should consider throwing all of this away and just use std::vector instead, which handles all of these details for you:

#include <vector>

template<class T>
class DynArray
{
public:

  DynArray(int n = 2)
  {
    vec.reserve(n);
  }

  void push_back(const T& n)
  {
    vec.push_back(n);
  } 

  void pop_back()
  {
    vec.pop_back();
  }

  T& front()
  {
    return vec.front();
  }

  T& back()
  {
    return vec.back();
  }

  T& at(int i)
  {
    return vec.at(i);
  }

  T& operator[](int i)
  {
    return vec[i];
  }

  int size() const {
    return vec.size();
  }

  int capacity() const {
    return vec.capacity();
  }

private:
  std::vector<T> vec;
};

Upvotes: 1

Related Questions