C++ Where is the seg fault?

Question

I am currently tackling this assignment for my computer science class:

Make your own dynamic array template. It should allow creating contiguous arrays (filled with things of the same type) which you can extend without worrying about running out of space.

Do one version using malloc and free.

Do one version using new and delete.

My version using new and delete works flawlessly; however, in trying to convert my new/delete code to using malloc/free, I keep getting a seg fault. I have narrowed down the segfault (I think), to being in a single function: addData. Take a look at the code in my main I used to test this:

Array2<int> *testArray3 = new Array2<int>(5);
Array2<int> *testArray4;
testArray3->initArray();
testArray3->printArray();
testArray4 = testArray3->addData(7);
testArray4->printArray();

return 0;

This gives a seg fault; however, when I change it to this:

Array2<int> *testArray3 = new Array2<int>(5);
Array2<int> *testArray4;
testArray3->initArray();
testArray3->printArray();
testArray4 = testArray3; //->addData(7);
testArray4->printArray();
return 0;

There is no seg fault. This makes me believe the issue is in my addData function. Here is the code for that:

Array2<T> *addData(T dataToAdd){
    Array2 <T> *tmp;
    tmp->data = this->getData();
    Array2 <T> *newData;
    newData->data = (T *) malloc(sizeof(T)*(this->size + 1));

    for (int i = 0; i < tmp->getSize() + 1; ++i){
        if (i < tmp->getSize()){
            //newData->data[i] = tmp->data[i];
            newData->setData(tmp->getData()[i], i);
        }
        else{
            //newData->data[i] = dataToAdd;
            newData->setData(dataToAdd, i);
        }
    }

    free(tmp->data);
    free(this->data);
    return newData;
};

I am new to programming as a whole and have not completely wrapped my head around pointers and memory allocation, etc. Any advice you could give me would be greatly appreciated! In case you need to see the rest of the code, here is the entire file I coded my template in. Thank you so much for your time!

#include <iostream>
#include <string>
#include <cstdlib>
#include <sstream>
using namespace std;

template<typename T>
class Array2{
public:
    Array2(int size){
        this->size = size;
        data = (T *) malloc(sizeof(T)*size);

    };
    Array2<T> *addData(T dataToAdd){
        Array2 <T> *tmp;
        tmp->data = this->getData();
        Array2 <T> *newData;
        newData->data = (T *) malloc(sizeof(T)*(this->size + 1));

        for (int i = 0; i < tmp->getSize() + 1; ++i){
            if (i < tmp->getSize()){
                //newData->data[i] = tmp->data[i];
                newData->setData(tmp->getData()[i], i);
            }
            else{
                //newData->data[i] = dataToAdd;
                newData->setData(dataToAdd, i);
            }
        }

        free(tmp->data);
        free(this->data);
        return newData;
   };
    ~Array2(){
        free(this->data);  
    };
    void initArray(){
        for (int i = 0; i < this->size; ++i){
            //this->data[i] = i;
            this->setData(i, i);
        }
    };
    void printArray(){
        //ostringstream oss;
        string answer = "";

        for (int i = 0; i < this->size; ++i){
            //oss << this->data[i] + " ";
            cout << this->data[i] << " ";
        }

        //answer = oss.str();

        cout << answer << endl;
    };
    T* getData(){
        return this->data;
    }
    int getSize(){
        return this->size;
    }
    void setData(T data, int index){
        this->getData()[index] = data;
    }
private:
    int size;
    T* data;
};

Answer 1

Array2 <T> *tmp;

Allocates a pointer. This does not point the pointer at anything or allocate any storage for the pointer to point at. What it points at without being explicitly assigned is undefined. If you are lucky, and you are this time, tmp points at an invalid location and the program crashes. If you are unlucky, tmp points at some usable region of program memory and lets you write over it, destroying whatever information was there.

tmp->data = this->getData();

Attempts to access the data member at tmp, but fortunately for you the access is in invalid memory and the program comes to a halt. It also has tmp's data pointing at this's data, and that's a dangerous position to be in. Changes to one will happen to the other because they both use the same storage. Also think about what will happen to this->data if you free tmp->data.

Or perhaps I'm wrong and the halt is here for the same reason:

Array2 <T> *newData;
newData->data = (T *) malloc(sizeof(T)*(this->size + 1));

Both need to be fixed. tmp doesn't have to live long, so we can make it a temporary local variable.

Array2 <T> tmp;

Typically this will be created on the stack and destroyed when the function ends and tmp goes out of scope.

But this will not work because Array2's constructor requires a size so it can allocate the array's storage. You need to find out how big to make it. Probably something along the lines of:

Array2 <T> tmp(this->size + 1);

But frankly I don't think you need tmp at all. You should be able to copy the dataToAdd directly into newData without using tmp as an intermediary.

newData is eventually going to be returned to the caller, so it needs a longer scope. Time to use new.

Array2 <T> *newData = new Array2 <T>(this->size + 1);

And through the magic of the constructor... Wait a sec. Can't use new. That makes this hard. malloc doesn't call constructors, so while malloc will allocate resources for newData, it doesn't do the grunt work to set newData up properly. Rule of thumb is Never malloc An Object. There will be exceptions I'm sure, but you shouldn't be asked for this. I recommend using new here and politely telling the instructor they are on crack if they complain.

Anyway, new Array2 <T>(this->size + 1) will allocate the data storage for you with it's constructor.

There is an easier way to do this next bit

for (int i = 0; i < tmp->getSize() + 1; ++i){
    if (i < tmp->getSize()){
        //newData->data[i] = tmp->data[i];
        newData->setData(tmp->getData()[i], i);
    }
    else{
        //newData->data[i] = dataToAdd;
        newData->setData(dataToAdd, i);
    }
}

Try:

for (int i = 0; i < tmp->size; ++i){
    newData->data[i] = tmp->data[i]; // you were right here
}
newData->data[tmp->size] = dataToAdd;

And back to something I hinted at earlier:

free(tmp->data);
free(this->data);

Both tmp->data and this->data point to the same memory. To be honest I'm not sure what happens if you free the same memory twice, but I doubt it's good. Regardless, I don't think you want to free it. That would leave this in a broken state.

Recap and fixes

Array2<T> *addData(T dataToAdd)
{
    Array2 <T> *newData = new Array2 <T>(this->size + 1);

    for (int i = 0; i < this->size; ++i)
    {
        newData->data[i] = this->data[i];
    }
    newData->data[this->size] = dataToAdd;

    return newData;
};

This version leaves this intact and returns a newData that is one bigger than this. What it doesn't do is add anything to this. Which is goofy for a method named addData.

It also leads to stuff like this:

mydata = myData->addData(data);

which leaks memory. The original mydata is lost without deletion, resulting in a memory leak.

What I think you really need is a lot simpler:

Array2<T> & addData(T dataToAdd)
{
    this->data = realloc(this->data, this->size + 1);
    this->data[this->size] = dataToAdd;
    this->size++;
    return *this;
};

realloc effectively allocates a new buffer, copies the old buffer into the new one, and frees the old buffer all in one fell swoop. Groovy.

We then add the new element and increment the count of elements stored.

Finally we return a reference to the object so it can be used in a chain.

Usage can be

myData.addData(data);
myData.addData(data).addData(moredata);
myData.addData(data).printArray();

and if you have operator << support written

std::cout << myData.addData(data) << std::endl;

I'd go back over the new version of Array if I were you. Most of the bugs picked off here are conceptual errors and also apply to it. You might just be getting unlucky and it merely looks like it works. I just read C++ Calling Template Function Error. The posted solutions fixed the immediate problem, but did not touch the underlying memory management problems.

As for the rest of your class, I advice following the link and answering What is The Rule of Three? Because Array2 violates the heck out of it.