Problems with recursive structure

• 09-29-2012
heinz55
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.
• 09-29-2012
laserlight
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.
• 09-29-2012
std10093
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 :)
• 09-29-2012
laserlight
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.
• 09-29-2012
heinz55
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.
• 09-29-2012
std10093
Quote:

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 :)
• 09-29-2012
heinz55
Quote:

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.
• 09-29-2012
std10093
Quote:

Originally Posted by heinz55
You are correct, but traditional C does not support the "new" keyword.

You are correct too in this case :)
• 09-29-2012
qny
Quote:

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!
• 09-29-2012
std10093
Quote:

Originally Posted by qny
Then you get something that is neither C nor C++.
Happy coding!

He is correct too :)
• 09-29-2012
Quote:

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. :p :p :p
• 09-29-2012
heinz55
Quote:

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++.