Problems with recursive structure

Hello,

I'm trying to understand the logic of recursive structures (or is "linked list" much proper term?) and I have a problem with following code:

Code:

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


struct person
    {
    const char *first_name;
    const char *last_name;
    int age;
    struct person *next;
    };

typedef struct person PERSON;


void add_new_person(PERSON **p_obj, const char *first_name, const char *last_name, int age)
    {
    PERSON *p = (PERSON *)malloc(sizeof(PERSON));
    p = *p_obj;

    if(p->next != NULL)
        p = p->next;

    p->first_name = first_name;
    p->last_name = last_name;
    p->age = age;

    p->next = (PERSON *)malloc(sizeof(PERSON));

    return;
    }


void show_persons(PERSON **p_obj)
    {
    PERSON *p = (PERSON *)malloc(sizeof(PERSON));
    p = *p_obj;

    if(p != NULL)
        {
        while(p->next != NULL)
            {
            printf("%s %s (age: %d years)\n"
                , p->first_name
                , p->last_name
                , p->age
                );

            p = p->next;
            }
        }

    return;
    }


int main(void)
    {
    PERSON *nice_person = (PERSON *)malloc(sizeof(PERSON));

    add_new_person(&nice_person, "Donald", "Duck", 122);
    add_new_person(&nice_person, "Jack", "Sparrows", 32);
    add_new_person(&nice_person, "Bruce", "Wayne", 40);
    add_new_person(&nice_person, "Gregory", "House", 55);
    add_new_person(&nice_person, "Luke", "Skywalker", 20);
    add_new_person(&nice_person, "Hannibal", "Lecter", 65);

    show_persons(&nice_person);

    return 0;
    }

I was hoping it prints out all six of the names, but the actual outpot is:

Code:

Donald Duck (age: 122 years)
Hannibal Lecter (age: 65 years)

(the first and the last item from the list).

I can't figure out what am I doing wrong here.

Let's start with:

Code:

PERSON *p = (PERSON *)malloc(sizeof(PERSON));

So far so good, but you don't need to cast the return value of malloc, and it would be a little better to use sizeof(*p), e.g.,

Code:

PERSON *p = malloc(sizeof(*p));

Next, we see:

Code:

p = *p_obj;

This must be a mistake. You just made p point to some dynamically allocated memory, and now you assign to p. At the very least, this leads to a memory leak.

If you want to keep the parameters of add_new_person as-is, then you should be checking if *p_obj is a null pointer. If it is, then you assign p to *p_obj. Otherwise, you search for the last element in the linked list, and assign p to its next pointer. You should only call malloc once in add_new_person. In main, nice_person should be initialised as a null pointer.

By the way, you should free what you malloc.

I can not see why the malloc in start of main function is needed.I would suggest you to just declare the pointer and set it to NULL.

add_new_person adds a person (a node of the list in other words) in the end of the list...So maybe you should

Code:

p->next=NULL;

as the last node has never a next one

Another thing: show_persons should not be calling malloc at all. In fact, the p_obj parameter of show_persons should be a pointer to PERSON, not a pointer to a pointer.

Oh, and if it is not part of your requirements, then you should not typedef struct person to PERSON as fully capitalised names are conventionally reserved for macro names, or to name constants.

Thanks for the advices given so far. I'll try to follow at least most of them.

Just a brief note considering this line:

Code:

PERSON *p = (PERSON *)malloc(sizeof(PERSON));

Since I try to make my code compatible with both C and C++ compilers, casting the return value of malloc is actually needed.

Originally Posted by heinz55

Since I try to make my code compatible with both C and C++ compilers, casting the return value of malloc is actually needed.

Well in C++ actually you would use the keyword new instead of malloc

Originally Posted by std10093

Well in C++ actually you would use the keyword new instead of malloc

You are correct, but traditional C does not support the "new" keyword.

Originally Posted by heinz55

You are correct, but traditional C does not support the "new" keyword.

You are correct too in this case

Originally Posted by heinz55

... I try to make my code compatible with both C and C++ compilers ...

Then you get something that is neither C nor C++.
Happy coding!

Originally Posted by qny

Then you get something that is neither C nor C++.
Happy coding!

He is correct too

Originally Posted by qny

Then you get something that is neither C nor C++.
Happy coding!

Don't be harsh about it. You know if you want a half-monkey and half-human, you just get one of each species, cut them in half, and stitch one half from each species, together -- Bingo!

That's just common sense.

Originally Posted by qny

Then you get something that is neither C nor C++.
Happy coding!

I'm just trying to write C code that can be compiled with C++ compilers as well. Simple as that.
But if I write C++ then I try to keep it pure C++.