Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just got into structs and I decided to create a list using them:

#include<stdio.h>
#include<stdlib.h>
int main(void)
{
    char counter;

    struct list
    {

        int x;  
        struct list *pointer1;

    };


    struct list test;
    struct list *pointer2;

    pointer2=&test;

    for(counter=0;counter<5;counter++)
    {

        pointer2->pointer1=malloc(sizeof(struct list)); 
        pointer2=pointer2->pointer1;    

    }   
}

I was wondering if there is another way to do the exact same thing. Is the use of one pointer only possible? By the way, I am pretty sure I could do the same thing using an array of structs too. Any advice would be really helpful!

share|improve this question
add comment

1 Answer

This is actually rather unconventional code. The head node of this list, test, resides on the stack. All of the subsequent nodes are allocated from the heap using malloc(). Since you never call free() on the memory returned by malloc(), you have a memory leak. When you eventually try to fix your memory leak, you're very likely to get confused: you need to free() all the nodes except test.

Furthermore, linked lists are usually terminated by a NULL pointer. After this program finishes, though, the tail of the list is an uninitialized chunk of memory from malloc(), which you should consider to return random garbage. (In practice, you are likely to get lucky and receive pre-zeroed memory since your program is running on a virgin heap, but you should not count on that behaviour.)

I suggest that you build the linked list all on the heap. Also, build it backwards, starting from the tail, so that your pointer is always pointing to the head. When you're done, you'll be holding a pointer to the head, which is useful.

#include <stdlib.h>

int main(void)
{
    struct list
    {
        int x;  
        struct list *next;
    };

    /* Create the list */
    struct list *head = NULL;
    /* Just use an int for the counter. A char is just weird. */
    for (int counter = 0; counter < 6; counter++)
    {
        struct list *node = malloc(sizeof(*node));
        node->next = head;
        head = node;
    }

    /* Destroy the list */
    while (head) {
        struct list *node = head;
        head = head->next;
        free(node);
    }
    return 0;
}
share|improve this answer
    
since we are in code review site, wouldn't you want the int in the for loop to be unsigned? –  Omer Apr 2 at 14:39
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.