6

I'm encountering a weird error in some homework that has me making a stack with a linked list. All functions in my program work perfectly, even in the beginning when I have no nodes, or after deleting a few. But when I make nodes and delete them all so I'm back to zero, then every function causes a crash. I've tried researching the problem, but the solutions I've found look almost identical to what I've already got, so obviously there's something critical I'm not seeing.

Here's the node removal function (the one I suspect to be the culprit in all this)

void remove(node** root)
{   
    node* temp = *root;
    node* previous = 0;
    if(*root)
    {
        while((*root)->next)
        {
            previous = *root;
            *root = (*root)->next;
        }
        delete *root;
        *root = temp;
        if(previous)
        {
            previous->next = 0;
        }
    }
    else
    {
        std::cout<<"cannot delete items from empty list\n";
    }
}

Here's the node insert function

void insert(node** root)
{
    node* temp = *root;
    if(*root)
    {
        while((*root)->next)
        {
            (*root) = (*root)->next;
        }
        (*root)->next = new node;
        (*root)->next->data = getnum();
        (*root)->next->next = 0;
        *root = temp;
    }
    else
    {
        (*root) = new node;
        (*root)->data = getnum();
        (*root)->next = 0;
    }

}

I'm fairly sure that the issue is somewhere in the code I linked, but just in case it's not, here's a pastebin to the full assignment http://pastebin.com/AWtG4qjD

2
  • 1
    I don't see anything wrong. What does it do when you run it under valgrind? Style issues: don't use *root as your iteration variable, it's confusing. Especially don't do that when you need to reset it to the original value after your iteration. And especially don't call that original value "temp" (it's not a temporary anything, it's the actual list being modified!). Also: why are you iterating to the end of the just to insert or delete at the end? Why not store the list in the opposite order and operate directly on the head in constant time?
    – Andy Ross
    Jul 20, 2012 at 17:59
  • it's part of an assignment. What you suggested does sound like a better way to do things, but since this is my first linked list assignment, I suspect I'm being asked to do it a deliberately complicated way to help me learn it better. I'm actually a pretty new programmer, so I haven't really worked out how to do clean and neat looking code, so the style advice is very appreciated. On that same note, I don't actually know what valgrind is, but I'm googling it and it looks interesting
    – kamstack
    Jul 20, 2012 at 18:04

1 Answer 1

4

remove implementation is not correct.Suppose the list has one element . In this case temp will point to "non-existing memory" after you execute delete *root; However what you are doing is *root = temp; this way you cause root to point to invalid node. And this causes the weird behavior later The possible way to make your implementation correct is :

void remove(node** root)
{
    //TODO: your code here
    node* temp = *root;
    node* previous = 0;
    if(*root)
    {
        while((*root)->next)
        {
            previous = *root;
            *root = (*root)->next;
        }
        delete *root;
        if(previous)
        {
            *root = temp;
            previous->next = 0;
        }
        else {
            *root = NULL;
        }
    }
    else
    {
        std::cout<<"cannot delete items from empty list\n";
    }
}

But I don`t advise you to iterate the list with root pointer .Define some iterator and look for the end with it instead of changing *root

1
  • that seems to have been the problem, thanks a bunch for your tip!
    – kamstack
    Jul 20, 2012 at 18:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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