c - replace linked list function

can anyone tell me why the replace function doesn't work? say main called replace(1,2,list). It should search the nodes, and if the value of the node is 1, it should make a new node, with value 2, to replace it, then free up the memory allocated to the first node. I can't figure it out =(

typedef struct iNode
{
    int myInt;
    struct iNode* next;
} IntNode, *IntNodePtr;

IntNodePtr insert(int i, IntNodePtr p)
{
    IntNodePtr newp = malloc(sizeof(struct iNode));
    newp->myInt = i;
    newp->next = p;
    return newp;
}

IntNodePtr delete(int i, IntNodePtr p)
{
    /* End of list check */
    if(p == NULL)
        return NULL;

    /* Check if current node is the one to delete */
    if(p->myInt == i)
    {
        IntNodePtr temp;
        temp = p->next;

        free(p);
        return 开发者_开发百科temp;
    }

    p->next = delete(i, p->next);
    return p;
}

IntNodePtr replace(int i, int j, IntNodePtr p)
{
    if(p == NULL)
        return NULL;

    if(p->myInt == i)
        insert(j, p->next);

    free(p);

    p->next = replace(i, j, p->next);
    return p;
}

---

There are a couple of issues with your replace() function. (It looked fine to me in your last question).

1. You call insert() when you find the node that has i but you do nothing with the new node (what insert() returns). Basically the new node you are inserting is the new p so you should set p to it.

2. You free p and immediately attempt to set the next field of the p to a value. The location p was pointing to is invalid so you shouldn't be doing that. You should use a temporary variable to save the old p so you can free that later. It should only be done if you are actually replacing it so it should be part of the previous condition.

I think that should cover it. Basically the changes are:

/* if the current node contains the int I'm looking for... */
if(p->myInt == i)
{   /* ... the current node needs to be replaced */
    /* save the current node to delete later (2) */
    IntNodePtr oldNode = p;

    /* insert a new node making it the new current node (1) */
    p = insert(j, oldNode->next);

    /* free the old node (2) */
    free(oldNode);
}
/* and so on */

---

As Mark pointed out, your free(p) has undefined behavior. Hence, the next statement

p->next = replace(i, j, p->next);

becomes invalid since you are trying to assign p->next to a pointer value, but p->next memory location itself is not defined.

But why have you made this replace function recursive? A simple while loop will be sufficient.

IntNodePtr replace(int i, int j, IntNodePtr p) {
    if(p == NULL)
        return NULL;
    IntNodePtr prevPtr = NULL;
    while(p){
        if(p->myInt == i){
            IntNodePtr temp = insert(j, p->next);
            if(prevPtr)
                 prevPtr->next = temp;
            free(p);
            break;
        }
        prevPtr = p;
        p = p->next;
    }
}

And you are incorrectly using your insert function as well, since you are not connecting the previous node to the newly created node.