Wrong answer from class member function when using template with class in c++?

Question

I am trying to implement a class template for an Array class that should provide a method to count elements equal to a given element, but I get wrong results. This is my code:

main.cpp

#include <iostream>
#include "array.h"

int main() {
    int arr[] = {1, 2, 3, 2, 5, 2};
    char brr[] = {'a', 'b', 'c', 'd', 'a'};

    Array<int> A(arr, 6);
    Array<int> B(0, 7);
    Array<char> C(brr, 5);

    std::cout << A.getCount(2) << std::endl;
    std::cout << B.getCount(0) << std::endl;
    std::cout << C.getCount('a') << std::endl;

    return 0;
}

array.h

template <typename T> class Array {
    private:
    T* ptrStart;
    int size;

    public:
    Array() {
        this->ptrStart = nullptr;
        this->size = 0;
    }

    Array(T defaultValue, int size) {
        T arr[size];
        for (int i = 0; i < size; i++) arr[i] = defaultValue;
        this->size = size;
        this->ptrStart = arr;
    }

    Array(T* arr, int size) {
        this->size = size;
        this->ptrStart = arr;
    }

    int getCount(T);
};

template <typename T> int Array<T>::getCount(T element) {
    int count = 0;
    for (int i = 0; i < this->size; i++) if (this->ptrStart[i] == element) count++;
    return count;
}

expected output:

3 
7 
2

actual output:

3 
0 
2

Answer 1

Both of these functions are invalid:

Array(T defaultValue, int size) {
    T arr[size];
    for (int i = 0; i < size; i++) arr[i] = defaultValue;
    this->size = size;
    this->ptrStart = arr;
}

Array(T* arr, int size) {
    this->size = size;
    this->ptrStart = arr;
}

The first function sets the data member ptrStart to a local variable arr that will not be alive after exiting the function.

The second function should dynamically allocate an array and copy elements from the array pointed to by arr.

For example, the second function can be defined the following way:

Array( const T *arr, int size) : ptrStart( new T[size] ), size( size ) {
    for ( int i = 0; i < size; i++ )
    {
        ptrStart[i] = arr[i];
    }
    // or you can use standard algorithm std::copy
}

Also, you need to include a destructor and either make the class uncopyable or define at least the copy constructor and copy assignment operator.

Answer 2

This is an error

Array(T defaultValue, int size) {
    T arr[size];
    for (int i = 0; i < size; i++) arr[i] = defaultValue;
    this->size = size;
    this->ptrStart = arr;
}

The array arr only exists in the constructor. When the constructor exits the array no longer exists. But ptrStart is pointing to this array even after it has been destroyed. So you get unpredictable results.

In C++ you must always think about the lifetime of objects. Objects don't keep existing just because a pointer is pointing at them (this is different in some other languages). If you have a pointer pointing at an object which no longer exists, that is called a dangling pointer. In your code ptrStart is a dangling pointer (when you use this particular constructor).

Also T arr[size]; is not legal C++ since in C++ array sizes must be compile time constants and size is a variable.

Answer 3

Array(T defaultValue, int size) {
    T arr[size];
    for (int i = 0; i < size; i++) arr[i] = defaultValue;
    this->size = size;
    this->ptrStart = arr;
}

Here you set ptrStart to point at the local variable in a function. Once that function is done, the pointer is dangling and you have Undefined Behavior. Your program might crash, format your hard drive, output one or more wrong values or, possibly, output the right value.

Answer 4

Your constructor

Array(T defaultValue, int size) {
    T arr[size];
    for (int i = 0; i < size; i++) arr[i] = defaultValue;
    this->size = size;
    this->ptrStart = arr;
}

has at least 2 problems.

First T arr[size] is not standard C++ but uses a compiler extension. Also it is a local variable and this->ptrStart = arr; only assigns its adress to the member. Note that arr is an array, not a pointer, but it can decay to a pointer.

Answer 5

The problem lies in this function:

Array(T defaultValue, int size) {
    T arr[size]; // Create array on stack - it will be deleted when the function returns
    for (int i = 0; i < size; i++) arr[i] = defaultValue;
    this->size = size;
    this->ptrStart = arr; // Storing address to array which won't exist after this line
}

After the function returns the pointer this->ptrStart points to some random place in memory and at the point when you call B.getCount(0), it can be pretty much any data in there. Reading memory pointed by that dangling pointer is an Undefined Behavior. If you are unlucky, this can even cause a runtime exception, not only wrong result. (Or maybe it is lucky, at least the result isn't wrong ;) )

---

To fix that problem, you should use operator new to create the array. This way, the array will be created on heap instead of stack.

T arr[size]; // bad
T *arr = new T[size]; // good

Heap is not cleared when function returns. This mean it is safe to use array from heap outside of the function it was created in.

Unfortunately, it also mean you need to delete it manually in the destructor of Array.

~Array() {
    if (createdInConstructor)
        delete[] this->ptrStart;
}

createdInConstructor is a new bool field of the class so the destructor don't try to delete the array in case of objects A & C. You could also use a smart pointer to achieve that a little easier.