Removing a string - linked list

[johngoodman]

I'm trying to go search through my linked list for a passed string and if it matches, remove it...but obviously link everything back together properly. This is what I have so far but when i pass it to my display function, which is properly working, it goes into an endless loop. Any pointers greatly appreciated

void llRemoveString(LinkedList** ll, char* string) {
    LinkedList* newNode = (LinkedList*)malloc(sizeof(LinkedList));
    newNode->value = string;
    LinkedList* n = *ll;
    
    while(n->next != NULL) {
        n = n->next;
        if(n->next->value == newNode->value){
            newNode->next = n->next;
            n->next = newNode;
        }
    }
}

[Cat]

*Another* thread about the same thing? I don't know why I'm even bothering to try to help, but I'll be nice.

Why are you creating a new node in a function apparently designed to remove nodes? That looks a lot more like a function to insert a node than one to remove a node.

As to why it infinitely loops, you end up trying to do two insertions of newNode, and end up creating a circularly-linked list of one element, because if your if statement was ever true for one loop (which is less likely than you think because you're doing pointer comparison not string comparison) your if() statement is guaranteed to be true the next iteration of the loop as well, causing newNode to point to itself.

Even if you had those problems solved your list would be horribly broken if value was not unique in the list, because you'd try to insert the same new node more than once in the list.

[johngoodman]

*Another* thread about the same thing? I don't know why I'm even bothering to try to help, but I'll be nice.

Why are you creating a new node in a function apparently designed to remove nodes? That looks a lot more like a function to insert a node than one to remove a node.

As to why it infinitely loops, you end up trying to do two insertions of newNode, and end up creating a circularly-linked list of one element, because if your if statement was ever true for one loop (which is less likely than you think because you're doing pointer comparison not string comparison) your if() statement is guaranteed to be true the next iteration of the loop as well, causing newNode to point to itself.

Even if you had those problems solved your list would be horribly broken if value was not unique in the list, because you'd try to insert the same new node more than once in the list.

Ok, so in my if statement, how do I compare the value at n's next node to the passed string? Is there a function for that? I'm used to java I'm sorry, I don't know many of the functions in C quite yet

void llRemoveString(LinkedList** ll, char* string) {
    LinkedList* n = *ll;
     
    while(n->next != NULL) {
         if(n->next->value == *string){
             n->next = n->next->next;
         }
        n = n->next;
    }
}

[anduril462]

strcmp(3): compare two strings - Linux man page

Something like:

strcmp(n->next->value, string)

That will return 0 if they're equal, and non-zero if they're different. Note, you don't want the * in front of string. strcmp takes a char pointer, to the first char in the string. *string dereferences the pointer, giving you a single char. strcmp does not like that.

[johngoodman]

strcmp(3): compare two strings - Linux man page

Something like:

strcmp(n->next->value, string)

That will return 0 if they're equal, and non-zero if they're different. Note, you don't want the * in front of string. strcmp takes a char pointer, to the first char in the string. *string dereferences the pointer, giving you a single char. strcmp does not like that.

Awesome, that fixed it! Thank you guys for the help, I think I'm starting to pick up on this linked list stuff more now

[johngoodman]

How would I take care of these memory leaks then? I'd like to maybe create another function and pass "LL" to it, delete it, and free it. But not sure if that'd take care of all the leaks

[MacNilly]

How would I take care of these memory leaks then?

Simple answer: call free() once for each object returned from malloc(). Basically, you should free the node (and probably the string it holds) in the node removal function.

[johngoodman]

How would you go about deleting a linked list? just assign it to NULL?

[MacNilly]

Would assigning a pointer to NULL call free() once for every object allocated via malloc()? C is not Java. To delete a linked list you have to free all the nodes and (probably) what they contain. THEN you can set the head pointer to NULL to indicate empty list.