-1

I have functioning code for Speller (problem set 5), but check50 reports a memory leak from the line table[i] = n;

Compilation fails when trying to free the node in the unload function:

bool unload(void)
{   
    free(dictword_counter);
    free(n);
    return (!fclose(dictfile));
}
dictionary.c:180:10: error: use of undeclared identifier 'n'
    free(n);
         ^
1 error generated.
// From top of the dictionary.h file: 
// Dictionary word counter
unsigned int *dictword_counter;

bool load(const char *dictionary)
{
    // Allocate memory for dict_word counter;
    dictword_counter = malloc(sizeof(unsigned int));
    if (dictword_counter == NULL)
    {
        printf("Could not allocate memory to load dictionary file.\n");
        return 0;
    }

    // Open dictionary
    dictfile = fopen(dictionary, "r");
    if (dictfile == NULL)
    {
        printf("Could not open dictionary file.\n");
        return 0;
    }

    // Read each word from dictionary file and load to hash table

    // Also count the number of dictionary words, for later
    *dictword_counter = 0;

    // Allocate memory for new node: location 'B'
    // node *n = malloc(sizeof(node));

    char word_buffer[LENGTH];
    while (fscanf(dictfile, "%s", word_buffer) != EOF)
    {
        // Allocate memory for new node and check success (location 'A')
        node *n = malloc(sizeof(node));
        if (n == NULL)
        {
            printf("Failed to allocate memory to new node.\n");
            return 0;
        }

        // Define node word from the buffer
        strcpy(n->word, word_buffer);
        unsigned int i = hash(word_buffer);

        // printf("hash value (modulo N): %i\n", i);

        // Prepend new node to location of hash in the array:
        // 1. Point the node to the first node in the linked list
        n->next = table[i];

        // 2. Point the table array to the new node, which is now first in the linked list.
        table[i] = n;

        // Not " *dictword_counter++; ", which gives error "expression result unused".
        *dictword_counter = *dictword_counter + 1;

//    Free (location 2)
//     free(n);

    }
//    Free (location 1)
//    free(n);
        return 1;

}

Trying instead to free(n) in the load function after it is used causes the programme to stop functioning.

I also noticed moving node * n = malloc(sizeof(node)); to before the while loop (at location B) causes check50 to fail on 'handles most basic words' and 'handles substrings'.

Why is this?

EDIT: Information added that free(n); in unload function does not compile.

2 Answers 2

0

You can't free the memory until you are done using it. You shouldn't be freeing the memory for the nodes in the load function. You free them in the unload function. If you free them during load then you have null pointers in your table which is not what you want.

When your program later tries to access the table, the data will not be there. Think about this for a second, you create a node and then insert the pointer to the node into an array, then you delete the node, what does the pointer point to now? What have you accomplished? Why did you even create it if you were going to delete it before using it?

You can only delete memory when you are done using it. When are you done using it? After you've finished any and all references to the data that memory contains. If you free it before you are done using it you will segfault or encounter undefined behavior when you try to access this freed memory. That's why you are crashing.

None of the places you have considered as deletion options are correct. Please rewatch the video for this lesson, you don't seem to understand the main point it.

Let's paraphrase this to a simple analogy.

  • You create object a
  • You populate a with some values
  • You delete a

Now you can't refer to the data you populated a with, you might as well not have created it.

Contrast this with:

  • You create object a
  • You populate a with some values
  • You do a bunch of work that interacts with a
  • You decide your work is concluded
  • You delete a
9
  • It is perhaps not a surprise you have made many assumptions given the reason for free(n) is in the load function (and not the unload function) was missing. Commented Apr 23 at 20:13
  • You misspelled 'programme', by the way. Commented Apr 23 at 20:18
  • Program - US spelling. Programme - UK. Remember, this is a global forum. ;-)
    – Cliff B
    Commented Apr 23 at 22:05
  • You don't call free(n) in unload, you have to free the appropriate pointer. You are being too literal. I made an analogy. I never said to call free(n) I said free the memory. That will mean iterating through the list and calling free appropriately. I am trying to teach you to fish, not just giving you answers.
    – UpAndAdam
    Commented Apr 24 at 15:21
  • I made no assumptions. You definitively shouldn't be freeing the nodes you create in your load function that you are inserting into the table I made an analogy to point out the flaw in your logic.
    – UpAndAdam
    Commented Apr 24 at 15:26
0

Memory is dynamically allocated to new nodes, which are incorporated into the table by a pointer at table[hash(word)], or if there is a collision, by a pointer at the end (or prepending to the beginning) of the linked list.

As new nodes are created successively with the call node *n = malloc(sizeof(node));, a simple callfree(n) is not sufficient, as it would after allocating memory to an integer, running a process, and then freeing it again, e.g. int *a = malloc(sizeof(int)); a_process(); free(a);

Each node of a linked list should be freed in turn, at each point in the hash table.

6
  • this is also NOT what you are doing. You are always inserting at the front of the list.
    – UpAndAdam
    Commented Apr 24 at 15:27
  • So you havent said anything that disagrees with or contradicts what I've said, you've just expanded upon the exact code to implement the free process that needs to occur in unload.. If you already understand this then you already know why free(n) was wrong at ANY TIME in the code so either your question makes no sense. There is a key component you are missing which is object lifetime, now just how to free the stuff but when to free it.
    – UpAndAdam
    Commented Apr 24 at 15:38
  • Correction made regarding prepending nodes. I didn't already understand this, and your posts were not overly helpful. Commented Apr 24 at 18:59
  • *except about object lifetime just now. You have missed what I was struggling to understand. Commented Apr 24 at 19:05
  • you were trying to free objects inside the very function you were declaring them in. thats the problem i addressed. I can't answer a question you dont ask.
    – UpAndAdam
    Commented Apr 25 at 19:57

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .