1

If I enter the input for this code as 1 2 3 4 5 then Ctrl-D to end the program, it will print

0 --> 5 --> 4 --> 3 --> 2 --> , which is strange. I tried following a tutorial for building a linked list but I think i've done it slightly wrong. 

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

struct list 
{
   int a;
   struct list *next;
};
typedef struct list List;

int main (void)
{
   List *start, *end;

   end = (List*)malloc(sizeof(List));
   end = NULL;

   while(scanf("%d", &(start -> a )) == 1)
   {
      start = (List*)malloc(sizeof(List));
      start -> next = end;
      end = start; 
   }

   end = start;

   while(start)                                      
   {
      printf("%d --> ", start -> a);
      start = start -> next;
   }

   return 0;
}
  • I realise I shouldn't cast the return of malloc and should be checking the return of scanf! This is merely test code to learn how to build linked lists.
R Sahu
  • 204,454
  • 14
  • 159
  • 270
Finlandia_C
  • 385
  • 6
  • 19
  • You are entering in the value for "a" before start has any memory allocated to it, other than the pointer size – scerrecrow Nov 18 '15 at 17:48
  • 1
    `end = (List*)malloc(sizeof(List));` should be `start = malloc(sizeof(List));` – haccks Nov 18 '15 at 17:50
  • haccks is right. find out why casting malloc is not such a good idea: http://stackoverflow.com/questions/1565496/specifically-whats-dangerous-about-casting-the-result-of-malloc – ForeverStudent Nov 18 '15 at 18:05
  • I added the line 'i realise i shouldn't cast malloc' because everytime the questions descend into arguments about malloc lol. – Finlandia_C Nov 18 '15 at 18:15

2 Answers2

3

You have a memory leak here

end = (List*)malloc(sizeof(List));
end = NULL;

Your first start is getting lost.

You need to allocate start to end before you allocate memory for start in loop.

start = (List*)malloc(sizeof(List));
end = NULL;

while(scanf("%d", &(start -> a )) == 1)
{
   end = start;
   start = (List*)malloc(sizeof(List));
   start -> next = end;
}

I would like to add that, this is essentially filling the link list in reverse. Because the code fills the link list in reverse order from the beginning

Haris
  • 12,120
  • 6
  • 43
  • 70
2

You are indeed entering the data into the list in a way that the most recent entry ends up at the head of the list.

One way to fix this problem is to make end point to the last pointer to next, and set it to point to start at the beginning. Note that end is now a "double-pointer":

List *start = NULL, **end = &start;
int a; // read data into a local, not into the node
while(scanf("%d", &a) == 1) {
    // Make a new node
    List *node = malloc(sizeof(List));
    // set the data to what we just read
    node->a = a;
    // This is the last node, so next is NULL
    node->next = NULL;
    // end points to previous node's next, so add new node to it
    *end = node;
    // Finally, re-point end to new node's next
    end = &node->next;
}

Demo.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523