Why is my struct pointer creating function returning NULL because of a local declaration?

Question

I've decided to go through all of D&R's exercises and in doing so I have encountered a peculiar event. Based on the book's own addtree function I modified it for my own structure:

struct gnode {
    char **words;
    int count;
    struct gnode *left;
    struct gnode *right;
};

And it now is:

struct gnode *addtree(struct gnode *p, char *w) {
    int cond;

    if (p == NULL) {
        printf("init null node\n");
        p = (struct gnode *)malloc(sizeof(struct gnode));  
        //MISTAKE I WAS MAKING:
        // struct gnode *p =(struct gnode *)malloc(sizeof(struct gnode)); 
        //p would be NULL every time.
        p->count = 1;
        p->words = malloc(8); 
        p->words[0] = strdup2(w);
        p->left = p->right = NULL;
    } else
    if ((cond = compare(w, p->words[0])) == 0) {
        printf("comp hit\n");
        p->count++;
        p->words = realloc(p->words, p->count * 8);
        p->words[p->count] = strdup2(w);
    } else
    if (cond < 0)
        p->left = addtree(p->left, w);
    else
        p->right = addtree(p->right, w);

    return p;
}

I would like to know why if a local pointer with the same name as the argument is returned, it is NULL every time.

Answer 1

There are multiple issues in your code:

• when the tree is empty, the line in the original code struct gnode *p = (struct gnode *)malloc(sizeof(struct gnode)); allocates a new gnode object, but also defines a new identifier p with a local scope, effectively shadowing the argument name. Hence the argument variable p is not updated and ultimately the original value (a null pointer) is returned by return p; at the end of the function.

  You can prevent this type of silly mistake by increasing the compiler warning level: gcc -Wall -Wextra or clang -Weverything

• the allocation p->words = malloc(8); is not portable. You should use:

  p->words = malloc(sizeof(*p->words));
  

• same for p->words = realloc(p->words, p->count * 8), it should be

  p->words = realloc(p->words, p->count * sizeof(*p->words));
  

• furthermore, since p->count was already incremented, setting the duplicate word should use:

  p->words[p->count - 1] = strdup2(w);
  

• why use strdup2(w) instead of strdup(w)?

• w should probably be defined as const char *w.

Here is a modified version:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct gnode {
    char **words;
    int count;
    struct gnode *left;
    struct gnode *right;
};

struct gnode *addtree(struct gnode *p, const char *w) {
    int cond;

    if (p == NULL) {
        printf("init null node\n");
        p = malloc(sizeof(*p));  
        p->count = 1;
        p->words = malloc(sizeof(*p->words)); 
        p->words[0] = strdup2(w);
        p->left = p->right = NULL;
    } else
    if ((cond = compare(w, p->words[0])) == 0) {
        printf("comp hit\n");
        p->count++;
        p->words = realloc(p->words, p->count * sizeof(*p->words));
        p->words[p->count - 1] = strdup2(w);
    } else
    if (cond < 0) {
        p->left = addtree(p->left, w);
    } else {
        p->right = addtree(p->right, w);
    }
    return p;
}