删除链表元素会导致无限循环

Question

我正在编写一个程序，它在输入中接受整数列表，并根据整数执行以下操作：

1. 如果输入中的值为负数，则删除绝对值

2. 如果是正数和偶数，则在列表顶部添加

3. 如果是正数和奇数，加到列表的尾部

4. 如果数字为零，则结束程序并打印列表。

我的问题在于 pop_el 函数，它会导致列表出现无限循环，因此当我打印列表时，程序会进入无限循环。 这是我的代码：

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

typedef struct ll_node_S * ll_node_ptr;
struct ll_node_S
{
    int v;
    ll_node_ptr next;
};
typedef struct ll_node_S ll_node;

ll_node_ptr push_tail(ll_node_ptr head, int v)
{
    ll_node_ptr backup = head;
    ll_node_ptr current = head;

    while(current->next != NULL)
    {
        current = current->next;
    }
    current->next = (ll_node_ptr) malloc(sizeof(ll_node));
    current->v = v;
    return backup;
}

ll_node_ptr push_head(ll_node_ptr head, int v)
{
    ll_node_ptr new_head = (ll_node_ptr)malloc(sizeof(ll_node));
    new_head->v = v;
    new_head->next = head;
    return new_head;
}

ll_node_ptr pop_el(ll_node_ptr head, int el)
{
    ll_node_ptr backup = head;
    ll_node_ptr current = head;
    ll_node_ptr previous = NULL;
    int found = 0;

    while(current != NULL && !found)
    {
        if(current->v == el)
        {
            if(previous == NULL)
            {
                backup = current->next;
                free(current);
                current = backup;
                previous = current;
            }
            else
            {
                previous->next = current ->next;
                free(current);
                current = current->next;
            }

            found = 1;
        }
        else
        {
            previous = current;
            current = current->next;
        }
    }
    return backup;
}

void print(ll_node_ptr head)
{
    ll_node_ptr current = head;
    printf("%d\n", head->v);
    while(current->next != NULL)
    {
        current = current->next;
        printf("%d\n", current->v);
    }   
}

int isPair(int n)
{
    return ((n % 2) == 0);
}

int main(int argc, char** argv)
{
    int n = 1;
    ll_node_ptr list = NULL;
    while(n != 0)
    {
        scanf("%d", &n);

        if(n < 0)
        {
            list = pop_el(list, -n);
        }
        else
        {
            if(isPair(n))
            {
                list = push_head(list, n);
            }
            else
            {
                list = push_tail(list, n);
            }

        }


    }

    print(list);
    //should free the list
    return 0;
}

这是测试用例（在输入中传递）我正在测试代码：

4
5
2
-4
-5
-3
9
2
0

应该产生以下输出：

2
2
9

有什么线索吗？

Answer 1

正如@Vitek 所指出的，您可以通过修复push_tail() 函数来解决您当前的问题。原始代码不仅将值存储在错误的节点中，而且未能将新分配的尾节点的next字段设置为NULL。这个函数还有一个问题：push_head() 和push_tail() 函数在使用NULL head 指针调用时都需要创建并返回一个节点指针。原始的 push_head() 函数执行此操作，但 push_tail() 函数不执行此操作。如果用户输入的第一个数字是奇数，则通过push_tail() 函数将第一个节点添加到列表中，从而导致未定义的行为（很可能是分段错误）。

更重要的是，您应该致力于简化您的代码。这将使它更容易编写，也更容易调试。例如，您的print() 函数可以简化为三行：

void print(ll_node_ptr current)
{
    while(current) {
        printf("%d\n", current->v);
        current = current->next;
    }
}

可以做几件事来改进push_tail() 功能。这是一个新版本：

ll_node_ptr push_tail(ll_node_ptr head, int v)
{
    ll_node_ptr current = head;
    ll_node_ptr prev = NULL;
    ll_node_ptr tail = malloc(sizeof(ll_node));

    if (tail == NULL) {
        fprintf(stderr, "Tail node allocation error\n");
        exit(EXIT_FAILURE);
    }

    tail->v = v;
    tail->next = NULL;

    while (current) {
        prev = current;
        current = current->next;
    }

    if (head) {
        prev->next = tail;
    } else {
        head = tail;
    }

    return head;
}

您不需要转换malloc() 的结果。 Opinions differ on whether or not you should do so，但不使用演员表编写会简化代码。此外，您应该检查以确保malloc() 已成功分配请求的内存。如果您改用realloc()，这一点尤其重要，因为不这样做会导致内存泄漏。此处新建尾节点后，立即设置v和next字段。

通过查看current 节点而不是查看current->next 节点来迭代链表通常看起来更简单。使用prev 节点指针有助于实现这一点。新函数遍历列表直到current == NULL，此时prev 指向列表的最后一个节点。如果head 作为NULL 指针传入（即列表为空，如果用户输入的第一个数字是奇数，则可能发生这种情况），则跳过迭代循环，因为current 已初始化到head。如果给函数一个非空列表，则循环后的代码将最后一个节点的next 字段设置为指向新创建的尾节点，否则head 成为新的尾节点。最后将head返回给调用函数。

pop_el() 函数可以进行很多简化，还有很多多余的代码。这个功能真的应该完全重新设计。你在哪里：

previous->next = current ->next;
free(current);
current = current->next;

您拥有freed 由current 引用的内存，然后您尝试取消引用指向该内存的指针。调用free() 后，您不再拥有此内存，这就是undefined behavior。相反，你想要：

current = previous->next;

但这并不重要，因为接下来将found 设置为1，循环终止，函数returns backup，不再使用current。您应该只删除上面对current 的分配，因为它不需要。

您应该能够使用这些信息来改进程序中的其余代码。您可能还想查看其他问题。 typedef 指针通常是一种不好的做法——这只会混淆代码并增加编程错误的可能性。您提供的规范没有明确说明如果有多个节点包含相同的值会发生什么，即应该删除一个还是全部？而且，如果用户输入的第一个数字是0怎么办？

Answer 2

几件事，

在pop_el，

1.如果previous 是NULL，那么您只需将head ptr 移动到下一个节点。这样它就会变成新的head。

if(previous == NULL)
{
    backup = current->next;
    free(current);      
    //current = backup;   ---> Not needed.
    //previous = current; ---> Not needed.
    break;  //            ---> No need of setting found flag. You can remove it
}

2.如果previous不是NULL那么你只需将previous节点下一个ptr指向current节点的下一个节点。

else
{
    previous->next = current ->next;
    free(current);          
    //current = current->next; ---> Not needed.
    break;  //            ---> No need of setting found flag. You can remove it
}

3.在push_tail 中，您正在为current->next 节点分配内存，在下一行中，您正在将v 添加到当前节点的v。那是错误的。检查以下，

ll_node_ptr push_tail(ll_node_ptr head, int v)
{
    ll_node_ptr backup = head;
    ll_node_ptr current = head;
    ll_node_ptr new = NULL; // Created new pointer
    while(current->next != NULL)
    {
        current = current->next;
    }
    //current->next = (ll_node_ptr) malloc(sizeof(ll_node));
    new = (ll_node_ptr) malloc(sizeof(ll_node));
    //current->v = v;   ----> incorrect. new Value is actually replacing the old value.
    new->v = v;       // New value is added in the newly created node.
    new->next = NULL;
    current->next = new;
    return backup;
}

4.你可以改进你的print逻辑

void print(ll_node_ptr head)
{
    ll_node_ptr current = head;
    //printf("%d\n", head->v);
    while(current != NULL)
    {
        printf("%d\n", current->v);
        current = current->next;
    }   
}

Answer 3

首先，我建议在处理列表时使用递归函数。它更容易调试和理解。

我想我发现你的 push_tail 函数有问题：

current->next = (ll_node_ptr) malloc(sizeof(ll_node));
current->v = v;

您为 current->next 分配内存，但将值分配给当前节点。 这应该是

current->next = (ll_node_ptr) malloc(sizeof(ll_node));
current->next->v = v;
current->next->next = NULL;

也许这与你的问题有关。