How can I prevent these memory leaks?

Question

I met huge problem with memory leaks and I don't know where to put that "delete" to get rid of them. Below is part of my code, and there is a full one: https://pastebin.com/Wtk83nuH.

string* startowa(int& rozmiar)
{
rozmiar = 5;
string* tablica = new string[rozmiar];

for (int i = 0; i < rozmiar; i++)
    tablica[i] = "text";
return tablica;
}

string* plusx(string* tab, int& rozmiar)
{
string tekst = "something";
string* tablica_3 = new string[rozmiar];
tablica_3[rozmiar - 1] = tekst;
for (int i = 0; i<rozmiar - 1; i++)
    tablica_3[i] = tab[i];

return tablica_3;
}

string* minusx(string* tab, int& rozmiar)
{
string* tablica_3 = new string[rozmiar];
for (int i = 0; i < rozmiar; i++)
    tablica_3[i] = tab[i];

return tablica_3;
}

int main()
{
int wybor = 1, rozmiar = 1;
string *tablica = startowa(rozmiar);

while (wybor != 55) {
    cin >> wybor;
    if (wybor == 1) {
        rozmiar++;
        tablica = plusx(tablica, rozmiar);
    }
    if (wybor == 6) wybor = 55;
    else {
        rozmiar--;
        tablica = minusx(tablica, rozmiar);
    }
    // there were other "ifs" but its just a part of the code
}
for (int i = 0; i < rozmiar; i++)
    cout << tablica[i] << endl;

delete[] tablica;
cin >> wybor;

getchar();

return 0;
}

Answer 1

The correct answer is use std::vector. For example:

vector<string> startowa(int& rozmiar)
{
    rozmiar = 5;
    vector<string> tablica(rozmiar);

    for (int i = 0; i < rozmiar; i++)
        tablica[i] = "text";
    return tablica;
}

Note the return by value. Don't fall into the trap of thinking you're saving processing time by returning by reference. That vector goes out of scope and is destroyed at the end of the function. With a returned reference the best you can hope for is the caller receiving a load of garbage and crashing before any damage can be done.

A decent compiler will eliminate the copying when you return the vector by value, and if the compiler decides that it cannot, std::move will take care of that.

vector also knows how big it is, eliminating the need for rozmiar.

Now... What went wrong? Let's look at the code

int main()
{
    int wybor = 1, rozmiar = 1;
    string * tablica = startowa(rozmiar); 

startowa allocated an array of strings and stored a pointer to the array in tablica.

    while (wybor != 55)
    {
        cin >> wybor;
        if (wybor == 1)
        {
            rozmiar++;
            tablica = plusx(tablica, rozmiar);

plusx allocated a new array of strings, a pointer to which has been returned and written over the pointer returned by startowa. startowa's array is now effectively lost, leaked, as it is next to impossible to find again to delete[].

We would need to delete[] tablica; before making the assignment. Clearly we can't do this before calling plusx as tablica is a parameter, so we need to store a temp.

            string * temp = plusx(tablica, rozmiar);
            delete[] tablica;
            tablica = temp;

But what if something unexpected happens and an exception is thrown? The code never hits the delete[] and BOTH allocations are lost. vector handles all this for you.

And back to the code

        }
        if (wybor == 6)
            wybor = 55;
        else
        {
            rozmiar--;
            tablica = minusx(tablica, rozmiar);

Same problem and solution as above.

        }
        // there were other "ifs" but its just a part of the code
    }
    for (int i = 0; i < rozmiar; i++)
        cout << tablica[i] << endl;

    delete[] tablica;

One of an in-determinant number of allocations is released here. The rest are lost.

    cin >> wybor;

    getchar();

    return 0;
}

Answer 2

To prevent memory leaks like that, avoid manual memory management. There are a lot of tools available to you.

For example, take your string array:

string* startowa(int& rozmiar) {
    rozmiar = 5;
    string* tablica = new string[rozmiar];

    // ...
}

This should be replaced by std::vector. And since a vector keep track of it's size, you don't need to pass the size as reference:

 std::vector<std::string> startowa() {
    // ...
    std::vector<std::string> tablica(5);

    // ...
}

Then, your function that operates on the array should take the vector by reference to about copies, and return another vector. Since a vector already has a function that insert a new element, your plusx function becomes this:

void plusx(std::vector<std::string>& tab) {
    std::string tekst = "something";
    tab.emplace_back(std::move(tekst));
}

And your minusx function becomes that:

void minusx(std::vector<std::string>& tab) {
    tab.pop_back();
}

By the way, with a vector, you can completely remove your startowa function by replacing the call in your main by this:

// Was `string *tablica = startowa(rozmiar);`
std::vector<std::string> tablica(5, "text");

Since std::vector manages it's memory itself, you don't need to delete it anywhere.

---

If you don't want to use vector, you can alway use std::unique_ptr<std::string[]>. The only difference in you code would be to send tablica.get() to your functions, and use std::make_unique<std::string[]>(rozmiar) instead of new std::string[rozmiar]

Answer 3

The memory leak is your least problem in that source code. In fact, you don't need heap allocations at all in your example.

Here are some fast improvements:
- use "std::string" instead of just string, I guess you are using "using namespace std"
- do not return a pointer to string, you can just declare a string and return it
- do not use a reference to an int as a function parameter if you are not returning it
- use const as much as you can
- replace "string *" with "const string&" if you are not returning it
- do not allocate string on heap (with new), instead declare it on stack
- use vectors

You can use this great site and Scott Meyers books for other C++ good practices.