CS50 Pset5 Speller - Valgrind error: Conditional jump or move depends on uninitialised value(s)

Question

When I run this code I get the errors:

==25659== ERROR SUMMARY: 1000 errors from 1 contexts (suppressed: 0 from 0)

with details from help50:

==25659== Conditional jump or move depends on uninitialized value(s)

Looks like you're trying to use a variable that might not have a value? Take a close look at line 140.

Line 140 belongs to unload function. Here's my code:

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <stdbool.h>
#include <cs50.h>
#include <string.h>
#include <strings.h>

#include "dictionary.h"

// Represents a node in a hash table
typedef struct node
{
    char word[LENGTH + 1];
    struct node *next;
}
node;

// Assign number of buckets in hash table
const unsigned int N = 1000;

// Hash table
node *table[N];

// Initialize hash table word count
int counter = 0;

// Returns true if word is in dictionary, else false
bool check(const char *word)
{
    // TODO
    int index = hash(word);

    // check if word is in dictionary
    node *cursor = table[index];

    // Traverse linked list, looking for word
    while (cursor != NULL)
    {
        if (strcasecmp(word, cursor->word) == 0)
        {
            return true;
        }

        cursor = cursor->next;
    }

    return false;
}

// Hashes word to a number
unsigned int hash(const char *word)
{
    // Sum up ASCII values of all characters in the word
    long sum = 0;
    int index = 0;
    for (int i = 0; i < strlen(word); i++)
    {
        sum += tolower(word[i]);
    }
    index = sum % N;
    return index;
}

// Loads dictionary into memory, returning true if successful, else false
bool load(const char *dictionary)
{
   // Open dictionary file
    FILE *current_dictionary = fopen(dictionary, "r");

    // Check if return value is NULL
    if (current_dictionary == NULL)
    {
        printf("Could not open %s\n", dictionary);
        return false;
    }

    // Read strings from file one at a time until the end of file
    char word[LENGTH + 1];

    while (fscanf(current_dictionary, "%s", word) != EOF)
    {
        // Create a new node for each word
        node *new_word = malloc(sizeof(node));

        // Check if return value is NULL
        if (new_word == NULL)
        {
            unload();
            return false;
        }

        // Copy and insert node into hash table
        strcpy(new_word->word, word);

        // Hash word to obtain a hash value
        int index = hash(word);

        // Initialize head to point to hash table index
        node *head = table[index];

        if (head == NULL)
        {
            table[index] = new_word;
            counter++;
        }
        else
        {
           new_word->next = table[index];
            table[index] = new_word;
            counter++;
        }
    }

    // Close file
    fclose(current_dictionary);
    return true;
}

// Returns number of words in dictionary if loaded, else 0 if not yet loaded
unsigned int size(void)
{
     return counter;
}


// Unloads dictionary from memory, returning true if successful, else false
bool unload(void)
{
    for (int i = 0; i < N; i++)
    {
        node *cursor = table[i];

    // Free linked lists
        while (cursor != NULL) // line 140
        {
            node *tmp = cursor;
            cursor = cursor->next;
            free(tmp);
        }
    }

    return true; 
}

I don't know the context of code but it seems u can at least add C Language tag to the question so u can have more views — M.Mavini, Jul 06 '22 at 18:30
Rather than putting "solved" in the title, [accept the answer](https://stackoverflow.com/help/accepted-answer) that addressed your question. — dbush, Jul 08 '22 at 01:21

score 1 · Accepted Answer · answered Jul 07 '22 at 09:45

1

Interesting, 1000 errors and 1000 buckets in the hash table.

One node in each hash bucket,the first one inserted, has an unitialized next. Subsequent inserts initialize it here new_word->next = table[index];

Program could use calloc when creating new_node to initialize all bytes to 0. Or set new_node->next to NULL after the node is (successfully) created.

answered Jul 07 '22 at 09:45

DinoCoderSaurus

6,110
2
10
15

Thanks @DinoCoderSaurus ! Per your suggestion, I set new_word->next to NULL and it fixed the problem. – concon817 Jul 08 '22 at 01:20
Although processors not setting a null-pointer to all-bits-zero are probably rare, [technically](https://c-faq.com/malloc/calloc.html); therefore, the the second suggestion to set them to null is probably more correct. – Neil Jul 08 '22 at 03:55
@Neil there are pink with purple poka-dot elephants, but those too are rarely seen. The definition of `NULL` is `0` (or all bits `0`). So, yes, in some world somewhere `NULL` may be defined differently, but it looks increasingly likely they have gone the way of the Dodo. – David C. Rankin Jul 08 '22 at 07:05
@DavidC.Rankin What I mean is with the language itself; [`(int)0` is not necessarily `(void *)0`](https://c-faq.com/null/machexamp.html). Though very commonly the same underlying bit-value, it is also confusing to read. I think OP has wisely chosen the 2nd option. – Neil Jul 09 '22 at 22:03
1

@Neil No disagreement there, since that is how a hashtable works. You declare your pointer array (or allocated block of pointers) and set each pointer `NULL` until you have something that hashes to that bucket. Absolutely no need to allocate storage for any bucket at all, you will simply assign it an address when your key hashes to that index. – David C. Rankin Jul 10 '22 at 02:47

CS50 Pset5 Speller - Valgrind error: Conditional jump or move depends on uninitialised value(s)

1 Answers1