Copy constructor not deep copying, error returning with array empty

Question

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;

*ptr is just the first element of the array, not the actual array. C arrays do not have value semantics and can't be copied by assignment anyway. — François Andrieux
– François Andrieux, Commented Apr 26, 2019 at 19:48
You may study the available implementations of std::vector before reinventing the wheel. — πάντα ῥεῖ
– πάντα ῥεῖ, Commented Apr 26, 2019 at 19:51
What is Size? It appears you are using its value in the copy constructor and copy assignment operator without copying the value from the source object. — 1201ProgramAlarm
– 1201ProgramAlarm, Commented Apr 26, 2019 at 19:51
Edit your question to include the definition of DynArray in your question, as it is essential to giving an accurate answer. — 1201ProgramAlarm
– 1201ProgramAlarm, Commented Apr 26, 2019 at 20:12

Remy Lebeau · Accepted Answer · 2019-04-26 23:58:08Z

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

Paul92 · Accepted Answer · 2019-04-27 00:32:22Z

0

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);

edited Apr 27, 2019 at 0:32

answered Apr 26, 2019 at 20:20

Paul92

9,1121 gold badge29 silver badges40 bronze badges

1 Comment

Remy Lebeau Over a year ago

You can't use memcpy() if T is not trivially copyable (ie, if it has user-defined constructors). You should take that suggestion out of your answer. You can safely replace the entire for loop with std::copy() always, as it works with non trivially copyable types, and may get optimized to a memcpy() for trivially copyable types.

Collectives™ on Stack Overflow

Copy constructor not deep copying, error returning with array empty

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related