Why this template class doesn't work properly?

Question

#include <bits/stdc++.h>
using namespace std;


template<class T>
class Array{
public:
Array(int sizeOfArray=1){
    arr = new T[sizeOfArray];
    for(int i=0;i<sizeOfArray;i++){
        arr[i] = 0;
    }
    maxSize = sizeOfArray;
    currentElement = 0;
}

T* growArray(){
    T* newarr = new T[maxSize*2];
    for(int i=0;i<maxSize;i++){
        newarr[i] = arr[i];
        }
    maxSize = maxSize*2;
    return newarr;
}

void add(T element){
    if(currentElement>=maxSize){
        arr = growArray();
    }
    arr[currentElement] = element;
    currentElement++;
}

T get(int index){
    return arr[index];
}

int nrElements(){
    return currentElement;
}

int length(){
    return maxSize;
}

~Array(){
    delete arr;
}
protected:

private:
T* arr;
int maxSize;
int currentElement;
};

template<class T>
Array<T> add(Array<T> &arr1, Array<T> &arr2){
int newsize = arr1.nrElements()+arr2.nrElements()+5;
int aux;
Array<T> arr3(newsize);
for(int i=0;i<arr1.nrElements();i++){
    aux = arr1.get(i);
    arr3.add(aux);
}
for(int i=0;i<arr2.nrElements();i++){
    aux = arr2.get(i);
    arr3.add(aux);
}
return arr3;
}

int main()
{
Array<int> arr(2);
for(int i=0;i<1000;i++){
    arr.add(i);
}

Array<int> arr2(2);
for(int i=1000;i<2000;i++){
    arr2.add(i);
}
Array<int> arr3;
arr3 = add(arr, arr2);
for(int i=0;i<arr3.nrElements();i++){
    cout<<arr3.get(i)<<" ";
}

return 0;
}

The function works properly. I mean on the line "aux = arr1.get(i);" is doing its job but when I try to read the elements of arr3 in main() the first two elements are not 0 and 1. The programs shows instead other big numbers like 175243462 and 152614213.

recommend against using a high level of caution with `#include using namespace std;` — user4581301, Dec 06 '16 at 17:45
Looks like you might have a Rule of Three failure. [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — user4581301, Dec 06 '16 at 17:51
You are leaking memory every time you do `arr = growArray()` — Chintan, Dec 06 '16 at 17:59
To get the result you expect, define Copy-Constructor & Assignment Operator (but there is still lot more you need to fix here, see/understand `std::vector` implementation) To avoid leaking memory, you need to `delete arr` before assigning it the newly allocated memory pointer. — Chintan, Dec 06 '16 at 18:05

score 3 · Answer 1 · edited May 23 '17 at 12:08

OP's reported bug is likely the result of a Rule of Three violation. What is The Rule of Three?

template<class T>
Array<T> add(Array<T> &arr1, Array<T> &arr2)
{
    int newsize = arr1.nrElements() + arr2.nrElements() + 5;
    int aux;
    Array<T> arr3(newsize); <<- made a local array
    for (int i = 0; i < arr1.nrElements(); i++)
    {
        aux = arr1.get(i);
        arr3.add(aux);
    }
    for (int i = 0; i < arr2.nrElements(); i++)
    {
        aux = arr2.get(i);
        arr3.add(aux);
    }
    return arr3; <<- return by value. May trigger a copy of arr3, then destroy arr3
}

There is no copy constructor or assignment operator, so the compiler generate default methods that are... Pretty simple. They blindly invoke the copy and assignment operator of their member variables, and the default copy behavior of a pointer is to copy the memory location and not the memory pointed at. This means the copies and the source's arr member all point at the same memory location. The Array destructor delete arr;, so the copies now are left pointing to freed memory. Accessing any of the copies is now undefined behaviour, and destroying any of the copies will cause a double delete. Very bad scene.

Solution, to this bug anyway, Implement copy constructor and assignment operator. Here is some help on that: Operator overloading

Bonus bug:

void add(T element)
{
    if (currentElement >= maxSize)
    {
        arr = growArray(); <<- did not release the old array pointed at by arr
    }
    arr[currentElement] = element;
    currentElement++;
}

add leaks memory because it does not delete arr before replacing arr This is more difficult to do in add (admittedly not by much) than it is in growArray so,

void growArray() <<- returns nothing
{
    T* newarr = new T[maxSize * 2];
    for (int i = 0; i < maxSize; i++)
    {
        newarr[i] = arr[i];
    }
    maxSize = maxSize * 2;
    delete arr; <<- clean up old arr
    arr = newarr; <<- assign arr here
}

and

void add(T element)
{
    if (currentElement >= maxSize)
    {
        growArray(); <<- everything done by growArray
    }
    arr[currentElement] = element;
    currentElement++;
}

Why this template class doesn't work properly?

1 Answers1