C++. How can I free memory correctly?

Question

Written code to find and remove the largest word in a string without the using of library functions. Everything works fine. But when I want to free memory, the result is negative (displays an empty line). If you remove the call to the memory release function, everything will work correctly, but there will be a leak of memory. How do I fix it? Please help me.

#include <iostream>

using namespace std;

int length(char *text) // string length
{
    char *begin = text;
    while(*text++);

    return text - begin - 1;
}

int size(char **text) // size of two-dimensional array
{
    int i = 0;
    while(text[i]) i++;

    return i;
}

void free_memory(char **text)
{
    for(int i=0; i<size(text); i++)
        delete text[i];

    delete [] text;
}


char **split(char *text, char delim)
{
    int words = 1;
    int len = length(text);

    for(int i=0; i<len; i++)
        if(text[i] == delim) words++;

    char **result = new char*[words + 1];
    int j = 0, t = 0;

    for(int i=0; i<words; i++)
    {
        result[i] = new char[len];

        while(text[j] != delim && text[j] != '\0') result[i][t++] = text[j++];

        j++;
        t = 0;
    }

    result[words + 1] = nullptr;

    return result;
}

char *strcat(char *source, char *destination)
{
    char *begin = destination;

    while(*destination) destination++;
    *destination++ = ' ';
    while(*source) *destination++ = *source++;

    return begin;
}

char *removeWord(char *in_string)
{
    char **words = split(in_string, ' ');
    int max = length(words[0]);
    int j = 0;

    for(int i=0; i<size(words); i++)
        if(max < length(words[i]))
        {
            max = length(words[i]);
            j = i;
        }

    int index;
    char *result;

    if(!j) index = 1;
    else index = 0;

    result = words[index];

    for(int i=0; i<size(words); i++)
        if(i != j && i != index)
            result = strcat(words[i], result);

    free_memory(words); // I want free memory here

    return result;
}

int main()
{
    char text[] = "audi and volkswagen are the best car";

    cout << removeWord(text) << endl;

    return 0;
}

Answer 1

In fact, this is C style programming - not C++. I see that your aim is to implement everything from scratch, possibly for practicing. But even then, your code is not designed/structured properly.

Besides that, you also have several bugs in your code:

1. result[words + 1] = nullptr; must be result[words] = nullptr;

2. You need result[i][t] = '\0'; after the while loop in split

3. delete text[i] must be delete [] text[i]

4. You cannot assign to your result pointer memory from words, then free it and then return it for use by the caller.

5. There is at least one further bug in the second half of removeWord. It would be tedious to try to understand what you are trying to do there.

You might want to start with a simpler task. You also should proceed step-by-step and check each function for correctness independently first and not implement everything and then test. Also take a look at the tool valgrind for memory checking - if you use Linux.

Answer 2

The way you free memory correctly is to use RAII:

• Only use new and new[] in constructors
• Pair those with delete and delete[] in the corresponding destructor
• Use automatic storage duration objects as much as possible

If you are specifically not using std::string and std::vector etc, for reasons of learning pointers, you will end up writing some small number of classes that resemble string and vector and unique_ptr, and then you go about programming as if you were using the std versions.

Answer 3

You have two issues. First is that result is assigned to a memory location in words. Second, is that you're storing the result of strcat in words[i] which will likely not have enough room (see strcat documentation).

result = new char[len(in_string)+1]; // +1 for space for null char

// the old loop reversed the word order -- if you want to keep doing
// that, make this a descending loop
for(int i=0; i<size(words); i++)
    if(i != j && i != index)
        strcat(result, words[i]);

free_memory(words);

return result;

So that when you free words, what result points to is also free'd. You would then need to free your result in main().

int main()
{
    char text[] = "audi and volkswagen are the best car";
    char * result = removeWord(text);
    cout << result << endl;

    delete[] result;

    return 0;
}