2

I am a beginner in C. I am working on an assignment to create a tree data structure. I have an if in main where I check if top is NULL before doing some action. Within my function to create the top pointer, it returns NULL right after being created (as expected, I haven't pointed it at anything yet). However when execution goes back to main, it returns an address even though I haven't assigned it one. So, my if doesn't work like I want it to.

I have noticed, if I set top to NULL from within main, then it sticks.

My question is, why does the address of top change from NULL to something else when execution returns to main? Have I done something in my code to unknowingly give point top to an unintended address?

//struct for org chart tree
    typedef struct org_tree{
        struct employee *top;
    } org_tree;

//create org tree
    void CreateTree(org_tree *t)
    {
        org_tree *temp;
        temp = malloc(sizeof(org_tree));
        t = temp;
        printf("%p\n", t->top); **here returns (nil) as expected
    }

//main program
    int main(void)
    {
        FILE *file;
        file = fopen("employee.list", "r");
        printf("file opened\n");
        org_tree t;

        CreateTree(&t);
        printf("%p\n", t.top) **here returns a memory location

        t.top = NULL **if I add this, then if below works.

        char mgr[20], st1[20], st2[20], st3[20], st4[20];

        while(fscanf(file, "%s %s %s %s %s\n", mgr, st1, st2, st3, st4)!=EOF)
        {
            employee *m;
            if (t.top !=NULL) **this if not working like I want it to because NULL doesn't "stick" unless set in main.
            {
             ///remaining code not relevant to question//
            }
         }
         ...
Anna
  • 53
  • 1
  • 7
  • Could it be that you need a pointer to the pointer? Maybe you'er only accessing a local copy of the pointer... meaning you can get what it's pointing to - but you wouldn't be able to change what it's pointing to? I dunno... – Gry- Nov 13 '17 at 04:37
  • Welp, re-read the question and now feel dumb. – Gry- Nov 13 '17 at 04:42
  • 1
    `CreateTree` is not setting `org_tree t` as you expect. Everything in C is pass by value. That means a local copy of `org_tree t` is made inside of `CreateTree`. You assign `t = temp`, but both of these go out of scope once `CreateTree` returns, and in fact creates a memory leak because nothing is pointing to the memory you `malloc`ed. If you want to save a pointer to this `malloc`ed memory, you must return it from the function, or pass in an `org_tree**` type and do `*t = temp;` – yano Nov 13 '17 at 04:45
  • Could you just try avoid using the temp and use "t" directly? @yano "Everything in C is pass by value" Are you sure about this? – zappy Nov 13 '17 at 05:04
  • @zappy yes I am. All function parameters in c are copied as if they are local variables. You can dereference pointers to create the effect of "pass by reference", but if you pass a pointer to a function, a local copy is made in that function and any changes you make to that pointer (or any other parameter) persist only in that function. – yano Nov 13 '17 at 05:09
  • @yano But in OPs code org_tree t; is not a pointer, and she is passing the address of it to CreateTree. So how a local copy is created there, i didn't understand ! – zappy Nov 13 '17 at 05:13
  • @zappy just the first return from a Google search, I'm sure you can find plenty of discussion about this online: https://denniskubes.com/2012/08/20/is-c-pass-by-value-or-reference/ – yano Nov 13 '17 at 05:13
  • 3
    @zappy the address of `t` is a pointer. In the function, a new `t` is created, which takes on the value of the `t` that was passed. Exactly the same as if you passed an int to a function. Any change you make to `t` in the function will not persist out side the function. There are 2 different `t`s, but they point to the same thing. So to change that thing you have to dereference the pointer. That is not happening anywhere in `CreateTree` – yano Nov 13 '17 at 05:18
  • 1
    `t ` is passed by value to `CreateTree()`. Assignment of `t = temp` therefore is not visible to the caller. The fact that `t` is a pointer doesn't change the fact it (the pointer passed) is passed by value. – Peter Nov 13 '17 at 05:22
  • @yano Thanks for your time and patience. why cant you write this as an answer? – zappy Nov 13 '17 at 05:30
  • 1
    But more to the point, you don't even need a `CreateTree` function! As soon as you do `org_tree t;`, you have an `org_tree` in automatic storage. No need to try to allocate memory for it, in fact it's a logical fallacy to even try. If you allocate memory for another `org_tree`, that's another `org_tree`, not the original `org_tree t`. @zappy no problem,, but I don't even know if this answers the question heh, I just jumped to the code and this was the first problem I saw. Besides, I'm on a phone right now,, not an ideal platform for answering. – yano Nov 13 '17 at 05:42
  • There is nothing special about pointers. A pointer parameter works exactly the same way as an `int` parameter. – molbdnilo Nov 13 '17 at 06:28
  • @yano very helpful comment thread. thank you! – Anna Nov 13 '17 at 18:15

4 Answers4

3

The thing is when you pass something to a function, a local copy of it is made. Now you are sending an address to the function. It is the content of t in CreateTree().

What is t in called function? t is a local variable to the CreateTree function. The type of t is org_tree* meaning it contains address of a org_tree variable.

You are allocating a piece of memory in CreateTree(). You are assigning the address of that chunk to the t variable. But that doesn't mean anything to the called function.

Local variable t's life ends once the called function's } is reached.

So back to main() it is still the old t variable that it was. Nothing(it's value) changed.


Now what are you trying to achieve here? Maybe you want to allocate a node dynamically.

Now let's look at an valid exmaple:-

    org_tree* t;

    CreateTree(&t);

    void CreateTree(org_tree **t)
    {
        org_tree *temp;
        temp = malloc(sizeof(org_tree));
        (*t) = temp;
    }

Now here can you tell what is there in t in called function now? It's still a pointer variable, having as content an address. Whose address? A pointer variable's address. More precisely org_tree* variable's address.

What does that mean? (*t) is basically the original t of callee- function. That means we can change it now. And that's what is being done. Here also if we do change value of t in callee function it is a change that will not stay in callee function. It is a local copy nothing more than that.


Is there a simple way to do this without **? There is.

    org_tree *t = CreateTree();

    org_tree * CreateTree(){
        org_tree *temp;
        temp = malloc(sizeof(org_tree));
        return temp;
    }

Here how is this working? Isn't temp a local variable? Yes it is. And after the function ends temp is out of life. But the memory address that it points to is not like that. That value is returned. Dynamically allocated memory is still there when the function CreateTree ends. And that memory is not de-allocated when the function is done execution.


Memory leak

Now look at the example (the very first one you wrote).

    void CreateTree(org_tree *t)
    {
        org_tree *temp;
        temp = malloc(sizeof(org_tree));
        t = temp;
        printf("%p\n", t->top); **here returns (nil) as expected
    }

Here you have assigned that allocated chunk's memory address to the local variable t in CreateTree(). CreateTree() ends and now is there anyway you can access that allocated memory? Nope.

That's why the previous function is said to be leaking memory. You call these 10000 times you will leak a large amount of memory.

Passing a pointer is nothing special. What makes them special is, we refer the address that it contains and the change is made to what is there in that address. And we start to think that it's pointer magic. It's a side effect that you exploit to persist change in value between functions.

Also here don't forget to check the return type of malloc and free the memory when you are done working with it.

user2736738
  • 30,591
  • 5
  • 42
  • 56
1

I would like to add a simple test case to demonstrate what others say.

#include <stdio.h>

void foo(int *p) {
  printf("pointer: %ld\n", (long)&p);
  printf("address: %ld\n", (long)p);
}

int main() {
  int i = 0;
  int* p = &i;
  printf("pointer: %ld\n", (long)&p);
  printf("address: %ld\n", (long)p);
  foo(p);
  return 0;
}

Results:

pointer: 140736715097888
address: 140736715097884
pointer: 140736715097848
address: 140736715097884

Above code prints the address of a pointer (where it is allocated) and contents of that pointer (the address where it points to) as you can see the content is the same but the addresses are different. That's why if you change p inside f that will have no effect on the outsider p. The pointer has been copied into f's stack.

sorush-r
  • 10,490
  • 17
  • 89
  • 173
0

It's because your top variable in the function is a local variable and it's scope is only valid in the function and is not available to other functions.

The variable top is redeclared n main function is a individual variable and is not related to the variable in the function. It has non null value cause it contains a garbage value.

To fix your problem you can make the top variable global . That is declare it once at before the functions . You can't have to declare it again in function and main function. Now this variable will shared by all the functions.

0

Most of the comments have picked up the various problems. See my comments inline with your original code, and a rewritten example below (using a list rather than tree for simplicity):

//struct for org chart tree
    typedef struct org_tree{
        struct employee *top;
    } org_tree;

//create org tree
    void CreateTree(org_tree *t)
    {
        org_tree *temp;
        temp = malloc(sizeof(org_tree));
        t = temp;
        printf("%p\n", t->top); //here returns (nil) as expected
        // Its not actually expected - the compiler happens to 
        // set it to null but it is actually undefined - you
        // have not yet set t->top to any address (through malloc)
        // or explicitly assigned it as NULL
    }

//main program
    int main(void)
    {
        FILE *file;
        file = fopen("employee.list", "r");
        printf("file opened\n");
        org_tree t; //This line creates the tree 
                    // as a locally allocated variable 
                    // of type org_tree. The declaration
                    // allocates it though it is yet 
                    // to be assigned.


        //CreateTree(&t);   //this is not required.
                            // It would be if your tree head
                            // was a pointer but it is not

        printf("%p\n", t.top); //**here returns a memory location
        // Through undefined behaviour. Even when you called
        // CreateTree, you reassigned it (t=temp) which 
        // creates a memory hole (the original address 
        // assigned by declaration is lost) 

        t.top = NULL; //**if I add this, then if below works.
        // Because it is now assigned.  

        char mgr[20], st1[20], st2[20], st3[20], st4[20];

        while(fscanf(file, "%s %s %s %s %s\n", mgr, st1, st2, st3, st4)!=EOF)
        {
            employee *m;
            if (t.top !=NULL) **this if not working like I want it to because NULL doesn't "stick" unless set in main.
            {
             ///remaining code not relevant to question//
            }
        }

And this is a re-written version which addresses the pointer issues by example (though noting that it's not the most elegant way to approach lists and trees - but will hopefully be useful for explanation). The immediately obvious opportunity is to combine CreateEmployee() and AddNodeToList():

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

//There's many more elegant ways to do this, though this
//version extends your approach to provide understanding

// This is also simplified as a linked list, unsorted
// rather than a tree - again just to look at the pointer
// concepts;

// To use a tree a recursive walk is best to avoid complex
// procedural structures plus logic to determine left / right
// branches etc.

typedef struct employee {
    char mgr[20], st1[20], st2[20], st3[20], st4[20];
    struct employee *next_node;
} employee;

//struct for org chart list
typedef struct org_list {
    struct employee *top;
} org_list;

//create org list
org_list *CreateList(void)  // must return it
{
    org_list *temp;
    temp = malloc(sizeof(org_list));
    //t = temp;

    temp->top = NULL; //needs to be explicit

    printf("%p\n", temp->top); //should be NULL

    return temp; //send back the address in the heap
}

//create employee
employee *CreateEmployee(employee *emp) {
    emp = malloc(sizeof(employee));
    emp->next_node = NULL;
    return emp;
}

int AddNodeToList(org_list* list_head, employee* e) {

    if (list_head->top == NULL) { //special case - empty list
        list_head->top = e;
        return 1; //all done
    } else {
        employee* temp_ptr;         //temporary pointer to walk the list
        temp_ptr = list_head->top;

        while (temp_ptr->next_node != NULL) {
            temp_ptr = temp_ptr->next_node;
        }
        // temp_ptr now points to the last node in the list
        // add the employee

        temp_ptr->next_node = e;
        return 1;
    }
}

//main program
int main(void) {
    FILE *file;
    file = fopen("employee.list", "r");
    printf("file opened\n");    //not necessarily - check (file != NULL)

    org_list *t;    //This line creates _a pointer to_ the list

    t = CreateList();   //you need an address to come back
    // The other way is to use a pointer to a pointer but this
    // gets confusing

    printf("%p\n", t->top); // This is now NULL
    // Note you haven't yet added an employee - just the head

    //t.top = NULL; //already done

    char mgr[20], st1[20], st2[20], st3[20], st4[20];

    while (fscanf(file, "%s %s %s %s %s\n", mgr, st1, st2, st3, st4) != EOF) {
        employee *m;    // this doesn't malloc or assign the underlying struct

        // this does
        m = CreateEmployee(m);

        if (m != NULL) {
            // copy the scanned strings into m->mgr m->st1 etc using strncpy

        }
        // but m is still not added to the list yet!
        AddNodeToList(t, m);
    }
}
BrendanMcL
  • 47
  • 1
  • 10