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.

On an unrelated note, in C [you don't have to (and really shouldn't) cast the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc/). — Some programmer dude, Jul 15 '22 at 13:48
The returned pointer is not NULL. But the parameter you are passing might be. Show [MCVE] if it is not the case. — Eugene Sh., Jul 15 '22 at 13:48
As for your problem and what I ***guess*** it might be, you have to remember that arguments to functions are passed *by value*. That means the value used in the call is *copied* into the functions local argument variable. That local variables lifetime ends when the function returns, so any changes you make to the variable (like assigning to it) will be lost when the function returns. — Some programmer dude, Jul 15 '22 at 13:50
This is for the same reason that `int foo(int a) { a = a+1; return a; }` and called like `foo(x)` will return a different value than will be in `x`. — Eugene Sh., Jul 15 '22 at 13:50

score 0 · Answer 1 · answered Jul 15 '22 at 13:54

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

You weren't returning a local variable.

You created a local variable, set it to allocated space, and used it to set some fields.

Then its scope ended at the end of the if block. It no longer shadowed the outer variable.

The outer variable p was never modified from its original null value.

You can avoid this by not shadowing variables.

Thank you! I completely forgot about scope. – pen_island Jul 15 '22 at 14:40 — pen_island, Jul 15 '22 at 14:40

score 0 · Accepted Answer · answered Jul 15 '22 at 14:27

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;
}

Thank you for the in-depth answer. Regarding malloc, I am not interested in portability at this time so I just used my machine's adress size. Strdup2 is also a modified function I made ( sorry for not providing context). I didn't know about the compiler commands, ty! — pen_island, Jul 15 '22 at 14:44
@pen_island: even if you are not concerned about portability, using the proper size improves readability and prevents silly mistakes. — chqrlie, Jul 15 '22 at 14:51

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

2 Answers2