Что не так в реализации моего метода?

Question

[HentaiEtoIskusstvo]

// Суть задачи заключается в том, что нужно создать метод, который будет удалять каждый элемент списка равного ключу

void List::DeleteIdenticalElements(char value) {
	Node* newnode = m_head;
	int i = 0;
	for (int i = 0; i < m_size; ++i) {		// Цикл, который отвечает за прохождение по каждому узлу
		if (value == newnode->m_data) {		// Если значение key равно значению m_data, тогда выполняется тело   
			if (i == 0) {					// Если i = 0, т.е. newnode = m_head, тогда...
				Node* temp = m_head;		// Указателю temp присваивается голова
				m_head = newnode->m_next;	// Голова смотрит на следующий после удаляемого узла узел
				delete temp;				// Удаление нужного узла
				--m_size;					// Уменьшение размера списка
				newnode = newnode->m_next;	// Теперь newnode равен следующему узлу
				continue;
			}
		}
		if (value == newnode->m_data) {		// Если значение key равно значению m_data, тогда выполняется тело
			Node* temp = m_head;
			Node* temp2 = nullptr;
			for (int j = 0; j < i - 1; ++j) {
				temp = temp->m_next;		// temp увеличивается до предстоящего к удаляемому узлу(-1) узла, чтобы запомнить узел, который будет смотреть на следующий после удаляемого узла узел
			}
			temp2 = temp->m_next;			// temp2 - указатель, который смотрит на удаляемый узел
			temp->m_next = temp2->m_next;	// Вместо удалямого узла temp смотрит на следующий
			delete temp2;					// Удаление нужного узла
			--m_size;						// Уменьшение размера списка
		}
		newnode = newnode->m_next;
	}
}

Выдает ошибку на этапе выполнения. Отладчик говорит, что ошибка в if (value == newnode->m_data). Но если поставить return до изменения счетчика цикла, то ошибки нет

Answer 1

[res2001]

Вы же умеете пользоваться отладчиком. Он вам лучше все скажет, чем кто-нибудь тут.
Предположительно выход за границу диапазона.
Возможно ошибка не в этом участке кода, здесь она просто проявляется.
Думаю, что при удалении элемента нужно декрементировать не только m_size, но и i.

PS: Я бы рекомендовал при создании нового узла обнулять указатель на следующий элемент, тогда определить конец списка можно просто по нулевому указателю и везде перед переходом на следующий элемент проверять не пустой ли он.
Заведите переменную для хранения предыдущего узла, тогда избавитесь от внутреннего цикла, а накладных расходов минимум.
Ну и код я бы немного реструктурировал, а то два одинаковых if идущих друг за другом режут глаз.
Комментарии, по большей части, "ни о чем".

Answer 2

[avtomh]

Если в списке больше одного удаляемого элемента, то уже при втором удалении цикл по j найдет "не тот" элемент, а следующий, т.е. который будет удаляться.
И, да, нуление указателя на m_next и указатель на предыдущий узел решают все проблемы.
Будет что-то вроде:

while(m_head && m_head->m_data==value)
{
Node *victim = m_head;
m_head = m_head->m_next;
delete victim;
}

if(m_head=NULL)
{
return;
}

Node *chkNode = m_head->m_next;
Node *prevNode = m_head;

while(chkNode)
{
if(chkNode->m_data==value)
{
prevNode->m_next = chkNode->m_next;
delete chkNode;
chkNode = prevNode->m_next;
}else{
prevNode = chkNode;
chkNode = chkNode->m_next;
}
}