CODEWITHSUNDEEP
cxcodelinked-list
Federico Chiesa
Federico Chiesa

Reputation: 75

Deleting a linked list node in a C function doesn't transfer to the calling function

I have this C function which is supposed to find an element in the linked list which has a specific "pos" value, delete it, and return the deleted value to the calling function. It does delete the item, but the change isn't saved in the calling function, the list just doesn't get updated with the new changes. My list is structured like this:

struct list{
    int value;
    int pos;
    struct list * next_ptr;
};

And my C function is this:

bool findDeleteElement(struct list **ptr, int position, int *value){
     struct list** temp = ptr;
    if(*ptr!=NULL){
        while((*ptr)->pos!=position) ptr=&(*ptr)->next_ptr; //Gets to desired node
        temp=ptr;
        value=&(*ptr)->value; //saves the value
        temp=&(*temp)->next_ptr; //Goes to next node
        ptr=temp; //Makes ptr point to next node
        return 1;
    }
    else return 0;
}

I just can't see what I'm missing. I'm a beginner so I probably made a simple mistake.

Upvotes: 1

Views: 61

Answers (3)

joop
joop

Reputation: 4523

int delete(struct llist **pp, int pos, int *result)
{
struct llist *tmp;

while ( (tmp = *pp)) {
        if (tmp->pos != pos) { pp = &tmp->next; continue; }
        *result = val;
        *pp = tmp->next;
        free(tmp);
        return 1;
        }
return 0; 
}

Upvotes: 0

meaning-matters
meaning-matters

Reputation: 22966

Change to:

*value = (*ptr)->value; //saves the value

You only set value, the local copy of your external variable's address. This does not change your external variable in the calling function.

Some question:

  • What happens when position has the wrong value, such that no node is found?
  • What's the purpose of temp = ptr;, because temp is overwritten by temp = &(*temp)->next_ptr; without having been used.

Disclaimer: I've not further checked this function.

I kindly advise you to take on other code formatting rules that add more air and make things more readable. Here's an example:

bool findDeleteElement(struct list **ptr, int position, int *value)
{
    struct list** temp = ptr;

    if (*ptr != NULL)
    {
        // Gets to desired node
        while((*ptr)->pos != position)
        {
            ptr = &(*ptr)->next_ptr; 
        }

        temp   = ptr;
        *value = (*ptr)->value;      // Saves the value
        temp   = &(*temp)->next_ptr; // Goes to next node
        ptr    = temp;               // Makes ptr point to next node

        return 1;
    }
    else
    {
        return 0;
    }
}

Upvotes: 1

JeremyP
JeremyP

Reputation: 86671

You are confused about pointers and dereferencing and what & and * actually do. This is a normal state of affairs for a beginner.

To start with, ptr and value when used without * preceding them are function arguments and like automatic (local) variables they disappear when the function scope exits. So this statement:

value=&(*ptr)->value;

Merely changes the value of value i.e. what it points to and has no visible effect to the caller. What you need to change is the thing that value points to. i.e. the statement should look like this:

*value = (*ptr)->value;

The difference is that instead of setting value to the address of (*ptr)->value it sets what valuepoints to to (*ptr)->value.

You have a similar problem with ptr. But your problems are more subtle there because you are also trying to use it as a loop variable. It's better to separate the two uses. I'd write the function something like this:

bool findDeleteElement(struct list **head, int position, int *value)
{
    struct list* temp = *head;
    struct list* prev = NULL;
    while(temp != NULL && temp->pos != position) 
    {
        prev = temp;
        temp = temp->next;
    }
    if (temp == NULL) // position not found
    {
        return false;
    }
    else 
    {
        *value = temp->value;
        // Now need to delete the node.
        if (prev != NULL)
        {
            // If prev has been set, we are not at the head
            prev->next = temp->next; // Unlink the node from the list
        }
        else // We found the node at the head of the list
        {
            *head = temp->next;
        }
        free(temp); // Assumes the node was malloced.
        return true;
    }
}

The above is not tested or even compiled. I leave that as an exercise for you.

Upvotes: 0

Related Questions