0

I wrote the function below to insert a value in order into the linked list, using a cmp function defined by the user. So i tested this out, I wrote my own cmp function for integers, and put in the values 9 and 1. The output was 9 and 1 (it did not insert into the linked list in order). After debugging this function for a while, I figured that if(prev == NULL) is never true, hence my program is breaking, is there anything I am doing wrong here???, we can compare to NULL right?

list_t *insert_in_order(list_t *list, void *value, int (*cmp)(void*,void*)) {

    node_t *new_node, *curr, *prev;
    new_node = (node_t *)malloc(sizeof(*new_node));
    assert(new_node != NULL && list != NULL);
    new_node->data = value;

    /*Special case when the list is empty*/

    if(list->head == NULL) {

        new_node->next = NULL;
        list->head = list->foot = new_node;

    }

    /*List is obviously not empty*/     

    else {

        curr = list->head;
        prev = NULL;

        /*Traverse the list*/
        while(curr) {

        /*I am basically going to break the loop when I find the right position*/
        /*to insert the node (after this node called prev)*/

            if(cmp(curr->data, value) > 0) {
                break;
            }
            prev = curr;
            curr = curr->next;
        }

        /*So now I know the node will go after the prev (defined above) node.*/

       /*Special case if this is the 0th position in the linked list i.e prev is null*/

        if(prev == NULL) {
            /*After doing some printfs here I see that prev is never null, anything*/
            /*wrong here???????????*/
            printf("prev is null\n");
            new_node->next = list->head;
            list->head = new_node;
        }

        else {
            printf("prev is not null\n");
            new_node->next = prev->next;
            prev->next = new_node;
        }
    }
    return list;
}
The Wizard
  • 943
  • 7
  • 21
  • 1
    Instead of just debug using `printf`, why not debug using a debugger? Step through the code, line by line, to see what *really* happens. – Some programmer dude Aug 07 '14 at 12:25
  • I would respectfully suggest that your cmp() function is broken. Can you print the result of the cmp function an ensure it's working as you intend? It's the only way I can see that you are getting the problem you describe. – Greycon Aug 07 '14 at 12:31
  • int cmp(void *a, void *b) { int *ad = a, *bd = b; if(ad < bd) return -1; else if(ad > bd) return 1; else return 0; } – The Wizard Aug 07 '14 at 12:34
  • 1
    You haven't dereferenced ad and bd there. – downhillFromHere Aug 07 '14 at 12:37
  • Ah, you are comparing the pointers, not the values. You should have; if(*ad < *bd), etc as ad is an "int *" – Greycon Aug 07 '14 at 12:38
  • Omg i am so embarassed, thanks so much it worked! Forgot the dereference! – The Wizard Aug 07 '14 at 12:39

1 Answers1

0

Insert in this code snippet

else {
    printf("prev is not null\n");
    new_node->next = prev->next;
    prev->next = new_node;

the following statement

else {
    printf("prev is not null\n");
    new_node->next = prev->next;
    prev->next = new_node;
    if ( new_node->next == NULL ) list->foot = new_node;

The other problem can be realted to the parameter value. It should be always allocated in the heap anew. You may not pass an address of the same local variable with different values. So the code should look for example as

int *p = ( int * )malloc( sizeof( int ) );
*p = 9;

insert_in_order( list, p, cmp );

p = ( int * )malloc( sizeof( int ) );
*p = 1;

insert_in_order( list, p, cmp );

Or you have to allocated a copy of the passed value in the function.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
  • Still the same output. – The Wizard Aug 07 '14 at 12:37
  • I really appreciate the help, but I had made a mistake in my cmp function, i didn't dereference the values before comparing them. But the parameter value, I stored it in an array, and I passed the address of the corresponding element. So I think that part is fine. Thanks =D – The Wizard Aug 07 '14 at 12:51