Inserting node in linked list

Question

I'm trying to insert a node dynamically between two indices which hold struct of type Node. The first element in array is head pointer and second element is tail.

I'm trying to dynamically grow the double linkedlist between the two indices of the array. Following is the code I've tried so far.

I could have created head and tail as an node dynamically as well but as per the requirement I've to do like this.

It is guarenteed to have the node which is to be inserted data value in between the value of qllentry[0].data and qllentry[1].data

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

struct Node {
    int data;
    struct Node *qprev;
    struct Node *qnext;
}Node;

struct Node qllentry[2];


int main()
{

struct Node head, tail;
    head.data = INT_MAX;
    tail.data = INT_MIN;

    head.qnext = &tail;
    tail.qprev = &head;
    head.qprev = NULL;
    tail.qnext = NULL;


    qllentry[0] = head;
    qllentry[1] = tail;
    int key = 20;
    struct Node *curr ;
    struct Node *prev;
    curr= &qllentry[0];

    while(curr->qnext != NULL && curr->data >= key) {
                curr = curr->qnext;
        }
    prev = curr->qprev;

    struct Node *new_node = (struct Node*)malloc(sizeof(struct Node));
        new_node->data = key;
        new_node->qnext = prev->qnext;
        prev->qnext = new_node;
        new_node->qprev = prev;
        if (new_node->qnext != NULL)
            new_node->qnext->qprev = new_node;


    return 0;
}

The insertion of the new node isn't happening between the head and tail indices as expected. I've added few print statements for debugging

Any help is appreciated.

David C. Rankin · Accepted Answer · 2018-02-18 04:48:36Z

While there is nothing wrong with keeping an array (or for that matter a pointer) that points to the head and tail of your list, if you use an array, after assigning the address keep your array references out of your list operations. Mixing &array[x] in with your list operations does nothing but cause confusion. When working with the list, treat it as a list and forget about the array.

Your primary problem is you iterate one node to far looking for where to insert the new_node resulting in you iterating to tail before you stop. Stop your iteration on the node before you insert new_node. You do this by testing for:

    /* test curr->qnext->data > key to stop before tail */
    while (curr->qnext && curr->qnext->data > key)
                curr = curr->qnext;

(note: masking levels of indirection with a variable like you do next with prev = curr->qprev; just hides details -- which can add to confusion later on. It's perfectly legal, but use with discretion...)

Now you can concentrate on inserting new_node between &head and &tail where it needs to go.

In any list insertion, you are simply re-wiring the pointer->next of the current node to point to the new_node and the pointer->prev of the next node to point to new_node. To finish the insertion your new_node->qprev points to curr and new_node->qnext points to curr->next, e.g.

    new_node->qprev = curr;         /* rewire pointers */
    new_node->qnext = curr->qnext;
    curr->qnext->qprev = new_node;
    curr->qnext = new_node;

(note: the easy way to figure it out is to pull at a piece of paper and a No. 2 pencil and draw a block for curr a block for new_node and a block for tail and then draw lines for prev/next pointers (for both the list without the new_node and with it). Then, with the logic straight, sit down to the keyboard and pecking it out.)

Further, you must always validate your allocations, e.g.

    /* allocate and VALIDATE! */
    if (!(new_node = malloc (sizeof *new_node))) {
        perror ("malloc - new_node");
        exit (EXIT_FAILURE);
    }

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed. So if you allocate it, keep track of a pointer to the block and free when you are done with it. For example, when done outputting the list values (or in a dedicated loop), you can free the memory you allocate similar to:

    curr = &head;                   /* output list */
    while (curr) {
        printf ("%d\n", curr->data);
        struct Node *victim = curr; /* self-explanatory */
        curr = curr->qnext;
        /* do not forget to free allocated memory */
        if (victim != &head && victim != &tail) {
            free (victim);
        }
    }

Putting it altogether, you can do something like the following:

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

struct Node {
    int data;
    struct Node *qprev;
    struct Node *qnext;
} Node;

struct Node qllentry[2];


int main (void) {

    struct Node head = { .data = INT_MAX }, 
                tail = { .data = INT_MIN },
                *curr,
                *new_node;

    qllentry[0] = head;     /* keep your array and list operations separate */
    qllentry[1] = tail;

    head.qnext = &tail;     /* begin list operations */
    tail.qprev = &head;

    int key = 20;

    curr = &head;

    /* test curr->qnext->data > key to stop before tail */
    while (curr->qnext && curr->qnext->data > key)
                curr = curr->qnext;

    /* allocate and VALIDATE! */
    if (!(new_node = malloc (sizeof *new_node))) {
        perror ("malloc - new_node");
        exit (EXIT_FAILURE);
    }

    new_node->data = key;           /* assign value to new_node */

    new_node->qprev = curr;         /* rewire pointers */
    new_node->qnext = curr->qnext;
    curr->qnext->qprev = new_node;
    curr->qnext = new_node;

    curr = &head;                   /* output list */
    while (curr) {
        printf ("%d\n", curr->data);
        struct Node *victim = curr; /* self-explanatory */
        curr = curr->qnext;
        /* do not forget to free allocated memory */
        if (victim != &head && victim != &tail) {
            free (victim);
        }
    }

    return 0;
}

Example Use/Output

$ ./bin/llarray
2147483647
20
-2147483648

Memory Use/Error Check

It is imperative that you use a memory error checking program to insure you do not attempt to access memory or write beyond/outside the bounds of your allocated block, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/llarray
==8665== Memcheck, a memory error detector
==8665== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==8665== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==8665== Command: ./bin/llarray
==8665==
2147483647
20
-2147483648
==8665==
==8665== HEAP SUMMARY:
==8665==     in use at exit: 0 bytes in 0 blocks
==8665==   total heap usage: 1 allocs, 1 frees, 24 bytes allocated
==8665==
==8665== All heap blocks were freed -- no leaks are possible
==8665==
==8665== For counts of detected and suppressed errors, rerun with: -v
==8665== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Simple Pointer Dump/Check

Lastly, in addition to stepping though the addresses with a debugger, you can always write a short debug routing to help you pick out if, and where, you have any problems with your pointer handling. (you don't have to output anything at all, you can just check the addresses with an equality if you like) This lets you look at all pointers at once. Just a simple routing to output your node pointers is often helpful. All you need is, e.g.

void debugptrs (struct Node *list)
{
    printf ("list pointers:\n\n");
    for (struct Node *iter = list; iter; iter = iter->qnext)
        printf ("prev: %16p    curr: %16p    next: %16p\n", 
                (void*)iter->qprev, (void*)iter, (void*)iter->qnext);
    putchar ('\n');
}

Which would provide output similar to:

$ ./bin/llarray
list pointers:

prev:            (nil)    curr:   0x7ffd56371910    next:        0x1038010
prev:   0x7ffd56371910    curr:        0x1038010    next:   0x7ffd56371930
prev:        0x1038010    curr:   0x7ffd56371930    next:            (nil)

I always found it helpful just to visually traverse the address from head to tail and back. If any prev or next for a node isn't what is output as the address for that node on the previous (or next) line, you know where you problem is.

Look things over and let me know if you have further questions.

Eric · Accepted Answer · 2018-02-18 04:06:47Z

1

Following is the code with some modification based on the code from the question, it prints the result as expected I guess:

dlink.c:

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

struct Node {
    int data;
    struct Node *qprev;
    struct Node *qnext;
} Snode;

int main() {
    struct Node *head = (struct Node*)malloc(sizeof(struct Node));
    struct Node *tail = (struct Node*)malloc(sizeof(struct Node));

    // init head,
    head->data = INT_MAX;
    head->qnext = tail;
    head->qprev = NULL;

    // init tail,
    tail->data = INT_MIN;
    tail->qprev = head;
    tail->qnext = NULL;

    int key = 20;
    struct Node *curr = head;
    struct Node *prev;

    //get the pointer of the process which has less priority than the current process
    while(curr->data >= key && curr->qnext != NULL) {
        curr = curr->qnext;
    }
    prev = curr->qprev;

    printf("head %p, data is %d, next is %p, prev is %p\n", head, head->data, (void *)head->qnext, (void *)head->qprev);
    printf("tail %p, data is %d, next is %p, prev is %p\n", tail, tail->data, (void *)tail->qnext, (void *)tail->qprev);
    printf("prev of new node %p, data is %d, next is %p, prev is %p\n", prev, prev->data, (void *)prev->qnext, (void *) prev->qprev);
    printf("--------------------\n\n");

    struct Node *new_node = (struct Node*)malloc(sizeof(struct Node));
    new_node->data = key;
    new_node->qnext = prev->qnext;
    prev->qnext = new_node;
    new_node->qprev = prev;

    if (new_node->qnext != NULL)
        new_node->qnext->qprev = new_node;
    else
        tail = new_node;

    printf("head %p, data is %d, next is %p, prev is %p\n", head, head->data, (void *)head->qnext, (void *)head->qprev);
    printf("new_node %p, data is %d, next is %p, prev is %p\n", new_node, new_node->data, (void *)new_node->qnext, (void *)new_node->qprev);
    printf("tail %p, data is %d, next is %p, prev is %p\n", tail, tail->data, (void *)tail->qnext, (void *)tail->qprev);

    return 0;
}

The running result:

head 0x2380010, data is 2147483647, next is 0x2380030, prev is (nil)
tail 0x2380030, data is -2147483648, next is (nil), prev is 0x2380010
prev of new node 0x2380010, data is 2147483647, next is 0x2380030, prev is (nil) // this is same as head,
--------------------

head 0x2380010, data is 2147483647, next is 0x2380460, prev is (nil)
new_node 0x2380460, data is 20, next is 0x2380030, prev is 0x2380010
tail 0x2380030, data is -2147483648, next is (nil), prev is 0x2380460

Suggestions

Don't mix struct (head, tail) & struct pointer (new_node), it's confusing, and easy to make mistake.
A singly linked list could be enough to do such insert, there is a tricky way to insert element in a singly linked list.
To get good performance, you can allocate a large cache, then create new node from the cache.
When compile your c code, add -Wall options, which will give you more warnings.

edited Feb 18, 2018 at 4:06

answered Feb 18, 2018 at 2:10

Eric

25.8k26 gold badges168 silver badges232 bronze badges

7 Comments

tourist Over a year ago

thank you for the suggestion but the main reason for using array was for O(1) time to access tail node , given the index array. And the code isn't working for the implementation with array. that is the reason I've posted it. It would be really helpful if you can help me

tourist Over a year ago

if you try adding head and tail pointers to an array and check the program, it isn't giving same result as we expect.

Eric Over a year ago

@naveenmarri There is a tail variable in your code, in your case it has value INT_MIN, which means the tail could change only when you insert a new node with data = INT_MIN, to avoid that simply change while(curr->qnext != NULL && curr->data >= key) { to while(curr->qnext != NULL && curr->data > key) {, the difference is to use > to replace >=. Then your tail node will never change, and you can always refer to it via the tail variable, that's O(1), thus you don't need that array at all.

tourist Over a year ago

but I want to insert this in a priority queue fashion, so that if elements happen to have same priority, the incoming node should insert at the end.

tourist Over a year ago

Also this logic I'm trying to implement for building an operating system, which has multiple of these kind of queues . Hence tracking of the tail pointers will be cumbersome. So I'm indexing in an array

|

Collectives™ on Stack Overflow

Inserting node in linked list

2 Answers 2

Comments

7 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

7 Comments

Your Answer

Sign up or log in

Post as a guest

Related