Search code examples
cpointersdata-structuresmalloctrie

Malloc after moving the pointer doesn't work in C?


I want to load a dictionary using TRIE. The first three parts of code worked as expected. But I tried to shorten the third part of the code, then it didn't work, not adding nodes to the TRIE.

Here is the declaration part:

#define ALPHABET_SIZE 27

// define a node for a TRIE.
struct node
{
    _Bool end_of_word;
    struct node *next[ALPHABET_SIZE];
};

// create a TRIE
struct node *root = malloc(sizeof(struct node));

// create a mover.
struct node *mover = root;

// read dictionary file
FILE *dictptr = fopen(dictionary, "r");

The main thing starts from here:

//load dictionary word by word;
char c;
while ((c = fgetc(dictptr)) != EOF)
{
    if (c == '\n')
    {
        mover->end_of_word = 1;
        mover = root;
    }

Here's where I want to optimize:

    else
    {
        if (c == '\'')
        {
            mover->next[ALPHABET_SIZE - 1] = malloc(sizeof(struct node));
            mover = &mover->next[ALPHABET_SIZE - 1];
        }
        else
        {
            mover->next[c - 97] = malloc(sizeof(struct node));
            mover = &mover->next[c - 97];
        }


        // check if memory allocation is successful.
        if (mover == NULL)
        {
            unload();
            fprintf(stderr, "unable to allocate memory to new node.\n");
            return false;
        }
    }

And here's what I optimized:

    else
    {
        if (c == '\'')
        {
            mover = &mover->next[ALPHABET_SIZE - 1];
        }
        else
        {
            mover = &mover->next[c - 97];
        }
        mover = malloc(sizeof(struct node));

Solution

  • By doing what you did you "detached" the target of assignment from the recipient lvalue in mover->next[...], thus destroying the original functionality of the code. In your version mover->next[...] remains unchanged.

    If one literally wanted to eliminate the code duplication here, one could've done something as follows

        struct node **pmover;
    
        if (c == '\'')
          pmover = &mover->next[ALPHABET_SIZE - 1];
        else
          pmover = &mover->next[c - 97];
    
        mover = *pmover = malloc(sizeof(struct node));
    

    This would be a literal implementation of your intent, which can also be rewritten as

        struct node **pmover = &mover->next[c == '\'' ? ALPHABET_SIZE - 1 : c - 97];
        mover = *pmover = malloc(sizeof(struct node));
    

    Although a better idea, in my opinion, would be

        struct node *new_node = malloc(sizeof(struct node));
    
        if (c == '\'')
          mover->next[ALPHABET_SIZE - 1] = new_node;
        else
          mover->next[c - 97] = new_node;
    
        mover = new_node;
    

    (or an equivalent ?:-based version).