Я работаю над заданием по реализации сортировки слиянием для односвязных списков на C++. Функция merge должна объединить два отсортированных списка на месте без создания новых узлов. Функция mergesort должна создать новый отсортированный список без изменения входного списка.
Моя реализация проходит все тестовые случаи, но имеет проблему с утечкой памяти, обнаруженную Valgrind. Вот соответствующий код с примером, запущенным в main внизу:
#include <cstdio>
#include <iostream>
/* node
The node type for our linked list.
*/
struct node {
int value;
node* next;
node(int val) : value(val), next(nullptr) {}
node(int val, node* next) : value(val), next(next) {};
};
/* merge
Merge the lists given by `left` and `right`, returning the head of the merged
list (the returned node should either be the head of `left` or the head of
`right`). The two input lists will always be sorted in ascending order.
Must run in O(m+n) time where `m` is the length of `left` and `n` is the
length of `right`.
Must use O(1) space (i.e., this is an in-place operation); no new nodes
may be created.
NOTE: This function should modify the `next` pointers in the nodes, but NOT
modify the `value`s. That is, it merges the lists by updating their list
structure, not by moving values from one list to another.
*/
node* merge(node* left, node* right) {
if (!left) return right;
if (!right) return left;
node* mergedHead = nullptr;
if (left->value <= right->value) {
mergedHead = left;
left = left->next;
} else {
mergedHead = right;
right = right->next;
}
node* mergedTail = mergedHead;
while (left && right) {
if (left->value <= right->value) {
mergedTail->next = left;
left = left->next;
} else {
mergedTail->next = right;
right = right->next;
}
mergedTail = mergedTail->next;
}
if (left) {
mergedTail->next = left;
} else {
mergedTail->next = right;
}
return mergedHead;
}
/* mergesort(input, length)
Recursively Mergesort the `input` list (whose length is given by `length`),
returning a new sorted list.
Must run in O(n log n) time, where n = `length`.
Must use O(n) space (returned list is created new).
NOTE: The `input` list must not be modified in any way.
*/
node* mergesort(node* input, int length) {
if (length <= 1) {
return new node(input->value);
}
int mid = length / 2;
node* left = input;
node* right = input;
node* prev = nullptr;
for (int i = 0; i < mid; ++i) {
prev = right;
right = right->next;
}
prev->next = nullptr;
node* sortedLeft = mergesort(left, mid);
node* sortedRight = mergesort(right, length - mid);
return merge(sortedLeft, sortedRight);
}
/* mergesort(input)
Mergesort the list whose head is `input`, returning a new sorted list. This
function should compute the length of the list, and then pass `input` and
the length to the two-parameter recursive `mergesort` overload, below.
Must run in O(n log n) time (but that's because `mergesort(node*,int)`, below
runs in O(n log n)).
Must use O(n) space (i.e., the returned list is created new).
*/
node* mergesort(node* input) {
if (!input) return nullptr;
int length = 0;
node* temp = input;
while (temp) {
++length;
temp = temp->next;
}
return mergesort(input, length);
}
void printlist(node *head) {
while (head) {
std::cout << head->value << " ";
head = head->next;
}
std::cout << "\n";
}
int main(){
// Example input: 4->1->3->2
node *input = new node(4, new node(1, new node(3, new node(2))));
node *sorted = mergesort(input);
printlist(sorted); // 1 2 3 4
return 0;
}
Вот что сообщает Валгринд:

Я не смог найти, где у меня происходит утечка памяти. Программа возвращает правильные результаты для тестовых случаев, но я не могу обнаружить ошибку в моей логике в отношении утечки. Что мне не хватает?
valgrind сообщает, что вы выделили память в функции make_list, которая находится в файле assign3_test.cpp в строке 292. Это будет первое место, которое стоит проверить.
Утечка памяти происходит из списков ввода, размещенных вне этого кода. Это предполагает отсутствие очистки в тестовом коде. Но я не думаю, что вас удовлетворил пункт «Список input нельзя изменять каким-либо образом». требование, так что, возможно, сначала исправьте это.
@trincot, как я это читаю, функция mergesort должна создавать одну копию входных данных («Необходимо использовать пространство O(n)»), merge, с другой стороны, должна быть на месте и ничего не выделять.
Ах да, разница в функциях. Ты прав, @teapot418
Значит, виноват prev->next = nullptr;. Вызывающий теряет доступ к оставшейся части входного списка и не может ее освободить.
Кроме того: проверка length <= 1 неверна: если вы вызываете сортировку слиянием для списка длиной 0, у вас неопределенное поведение.
На самом деле я бы сказал, что ошибка в задании. Комментарий к коду не отражает контракт API. Данная структура не является постоянной корректной. В частности, node* input должен быть указателем на константу node. Если бы входные данные были константными, компилятор не смог бы скомпилировать, что сделало бы ошибку в коде очевидной.
PS: Почему нет ~node()? Возвращаемое значение должно быть std::unique_ptr<node>. Семантика копирования/перемещения также должна быть определена (по крайней мере, удалена).





Проблема в следующем задании:
prev->next = nullptr;
Это изменяет входной список. Но в задании говорилось:
Функция
mergesortдолжна создать новый отсортированный список без изменения входного списка.
Благодаря рекурсии эта мутация в конечном итоге изолирует каждый узел во входных данных, так что после завершения вызова mergesort верхнего уровня указатель input будет просто указывать на узел, у которого нет преемника. Как следствие, вызывающая сторона не имеет возможности освободить память, занятую любым другим узлом, принадлежащим входному списку.
Прагматичный способ решить эту проблему — отменить эту мутацию перед возвратом (но смотрите дальше). Итак, измените это:
prev->next = nullptr;
node* sortedLeft = mergesort(left, mid);
node* sortedRight = mergesort(right, length - mid);
к этому:
prev->next = nullptr;
node* sortedLeft = mergesort(left, mid);
node* sortedRight = mergesort(right, length - mid);
prev->next = right; // restore!
Но поскольку ваша двухпараметрическая функция mergesort не зависит от проверки конца заданного списка на основе нулевого указателя, а только от заданной длины (и это хорошо!), вам на самом деле не нужно разбивать входной список на все, и можно просто не использовать prev.
Огромное спасибо! Вы сэкономили мне невероятное количество времени!!
@MiladKhazani Вы можете это увидеть, если поставите printlist(input) после сортировки
На самом деле сделать немутирующую сортировку слиянием очень просто, вы передаете узел «один за концом» вместо длины: coliru.stacked-crooked.com/a/1496f64b7f39b52d
@trincot - удалил мои предыдущие комментарии. Я понимаю, что базовый вариант всегда будет достигнут на каждом узле, поэтому prev->next = nullptr; не нужен. Моя точка зрения заключалась в том, что должно быть быстрее, чтобы избежать сканирования левого списка на наличие mid узлов, чтобы разделить список и вместо этого иметь указатели возврата или обновления слияния и сортировки слиянием для первого и конечного узлов, начиная с базового случая, и обновляться по мере развертывания цепочки вызовов. путем передачи счетчиков для слияния.
@trincot Ответ по-прежнему работает, спасибо. Я не уверен, почему его удалили. Я еще раз добавил согласие. Спасибо вам за помощь
Во-первых, это не минимально воспроизводимый пример. Где находится функция
main? Какие тестовые данные вы используете? Во-вторых, - но я не могу найти ошибку в своей логике - Используйте отладчик. В-третьих, я не смог найти ни одного вызоваdeleteв том, что вы опубликовали, учитывая, что в вашем коде естьreturn new node(input->value);.