Help with linked list

[Alien]

Hi I am having some problems with the annoying segmentation error when I try to run a program that creates a linked list of numbers that are passed on to it.

All I am trying to do is to create a link list which takes number that may not possibly be in order. For example, I try to read a file which says,

101
102
105
104
103
101 <-------- Duplication
It should create a linked list with ascending order. I wrote a but it gives me a segmentation fault and after spending many hours deskchecking the code, I couldnt find the bug. Can anyone help me out?

CODE BELOW
NB: this is just part of the code, but the error lies in the function addBlock().

struct block
{
   struct block *down;
   int number;   
};
typedef struct block block;
 
void addBlock(block **start, int number);
 
int main(int argc, char *argv[])
{
   block *head = NULL;
   /* Codes to read every line of a file and obtain the number and store in number variable below, goes through a loop
      to do this and loop goes on until the end of file is reach */
 
 
   int number = some Function that Reads Number Into It ;
   add(Block(&head, nuumber);
 
}
 
void addBlock(block **start, int number)
{
 
   /*
        There are 4 possibilities here: 
        1) temp is the first block
        2) temp can fit between 2 blocks,
        3) A block with same number already exists
        4) None of the above, set temp to the end of list         
   */
 
   block *temp = newBlock(number);
   block *curr = NULL;   /*curr scans every element in the link list*/ 
 
   if(*start == NULL)   /*First block*/
   {
      *start = temp;
   }
   else if ((*start)->down == NULL)      /*Second block*/
   {
       /*Check whether temp can go as first block*/
       if(temp->number < (*start)->number)
       {
           temp->down = *start;
          *start = temp;
       }
        else      /*Put it to the end of the list, as second element*/
           (*start)->down = temp;
   }
   else      /*If there is more than 2 blocks*/
   {
      /*Scan down the list*/
        for(curr = *start; curr->down != NULL; curr = curr->down)
        {
         /*Check if temp can fit in betwen any blocks*/            
            if(curr->number < temp->number && 
                                                    curr->down->number > temp->number)
            {
                temp->down = curr->down;
                curr->down = temp;
                break;
            }        
        }
   }
   /*Checks if the new block has been added or not, if not 
     it will be added to the end of the list */
   if(temp->down == NULL)
       curr->down = temp;    
}
 
 

[weaknessforcats]

block *temp = newBlock(number); <<<???
block *curr = NULL; /*curr scans every element in the link list*/

if(*start == NULL) /*First block*/
{
*start = temp;

First: Does newBlock create the block on the heap??
Second: Does newBlock set the down pointer to 0?

Third: The add logic seems overly complicated. I would expect the check for *start to be NULL. After that, I don't see why the concern about 2 blocks. That is to say, is *start is not NULL you compare numbers until you reach the insertion point.

This code:

/*Scan down the list*/
for(curr = *start; curr->down != NULL; curr = curr->down)
{
/*Check if temp can fit in betwen any blocks*/
if(curr->number < temp->number &&
curr->down->number > temp->number)
{
temp->down = curr->down;
curr->down = temp;
break;
}
}

assumes that curr->down is not zero. That's a bad assumption. curr->down will be zero when curr is the end of the list.

What you have to do is save the value of curr so that on each cycle of the loop the save value is the address of the previous block. Then when your
temp->number is greater than curr->number, you insert after the save, that is, before the current node. Of course, if you run off the end of the list you insert there. And always, be sure your curr->down is not zero before using it.

My guess is that a lot of the code in the add block funciton can be deleted.

[Alien]

Thank you weakness for looking into it.

New block just creates a new block (dunno on heap or not) the code for it below is:

block* newblock(int number)
{
   /* dynamicaly allocate memory for an intNode
      make sure it is initialised correctly
   */
 
   block *temp;
 
   temp = (block *) malloc(sizeof(block));
 
   if (temp == NULL)   /*checks whether allocation was success or not*/
   {
      printf("WARNING - Memory allocation error\n");
      exit(EXIT_FAILURE);
   }
   else
   {
      temp->number = number;
      temp->next = NULL;
      temp->down = NULL;
   } 
   return temp;   
}
 

It just uses malloc and returns a pointer to the block created. AddBlock function merely adds it.

So by complicated, are you suggesting that this functions is doing too much and should be split it's task into smaller functions? Something like,

NewBlock = mallocs and creates a new block.
AddBlock = adds it to the list and returns a pointer to the block added

and perhaps create another function say, orderBlocks which will order them in ascending order of the number?

Thank you for your help.

Regards.

[weaknessforcats]

So by complicated, are you suggesting that this functions is doing too much and should be split it's task into smaller functions? Something like,

NewBlock = mallocs and creates a new block.
AddBlock = adds it to the list and returns a pointer to the block added

It looked like the add function was larger than it needed to be. So I reworked it:

void addBlock(block **start, int number)
{
 
   block *temp = newBlock(number);
   block *curr = NULL;   /*curr scans every element in the link list*/ 
   block* prev = NULL;  /* used to save address of block before curr */
 
   if(*start == NULL)   /*First block*/
   {
      *start = temp;
      return;                   /*you are done */
   }
   /*Scan down the list*/
        prev = 0;
        for(curr = *start; curr != NULL; curr = curr->down)
        {
            /*Check if temp can fit in betwen any blocks*/      
            if(temp->number < curr->number )
            {
                if (prev)
                {
                      prev->down = temp;
                      temp->down = curr;
                }
                else
                {
                       temp->down = curr;
                       *start = temp;
                 }   
               prev = curr;    
}
 

I didn't debug this, but what I did was try to simplify the logic and keep the list sorted.

So, if the start is NULL then temp is your new start.
Otherwise, you scan the list comparing numbers as long as the temp->number ius greater than the curr->number. When temp->number is less (or equal) to curr->number, then you have gone one block too far. So you insert the new block after the previous block. Conveniently, you have saved the address of the previous block.

Again, I apologize for any errors as I am trying to communicate the logic flow.