Delete operation is abnormal when iterating elements

[yuantingzhong]

The following code is used to test:

m := orderedmap.NewOrderedMap()
	m.Set(10, "ten")
	m.Set(9, "nine")
	m.Set(8, "eight")
	m.Set(7, "seven")
	m.Set(6, "six")
	m.Set(5, "five")
	m.Set(4, "four")
	m.Set(3, "three")
	m.Set(2, "two")
	m.Set(1, "one")

	for el := m.Front(); el != nil; el = el.Next() {
		if el.Key.(int)%2 == 0 {
			m.Delete(el.Key)
		}
	}

	for el := m.Front(); el != nil; el = el.Next() {
		fmt.Println(el.Key, el.Value)
	}

with the output:

9 nine
8 eight
7 seven
6 six
5 five
4 four
3 three
2 two
1 one

The reason for this is

func (l *list) Remove(e *Element) {
	if e.prev == nil {
		l.root.next = e.next
	} else {
		e.prev.next = e.next
	}
	if e.next == nil {
		l.root.prev = e.prev
	} else {
		e.next.prev = e.prev
	}
	e.next = nil // avoid memory leaks
	e.prev = nil // avoid memory leaks
}

e.next and e.prev are assigned to nil.
I don't think it's necessary. So Under what circumstances will this phenomenon(memory leaks) take place?

[elliotchance]

You cannot modify a linked list while you're iterating it. The docs say this:

If the map is changing while the iteration is in-flight it may produce unexpected behavior.

To answer your question about memory leaks: it's probably not an issue, but it's better to be safe. I'm guessing at the time I was thinking about the case where more than one node was disconnected (takes more collection cycles to cleanup?) or if there was a case of a ring list (again, I think the Go garbage collector is smart enough to handle this) but it's basically no cost so I played it safe.

[yuantingzhong]

You cannot modify a linked list while you're iterating it. The docs say this:

If the map is changing while the iteration is in-flight it may produce unexpected behavior.

To answer your question about memory leaks: it's probably not an issue, but it's better to be safe. I'm guessing at the time I was thinking about the case where more than one node was disconnected (takes more collection cycles to cleanup?) or if there was a case of a ring list (again, I think the Go garbage collector is smart enough to handle this) but it's basically no cost so I played it safe.

Ok, I may delete elements in a for loop by other means to solve my problem. Thanks for your reply.

[elliotchance]

A linked list can be designed to support modification during iteration, its just that this implementation does not. Delete the elements after iterating:

	var deleteKeys []string
	for el := m.Front(); el != nil; el = el.Next() {
		if el.Key.(int)%2 == 0 {
			deleteKeys = append(deleteKeys, el.Key)
		}
	}

	for _, key := range deleteKeys {
		m.Delete(key)
	}

On a related note, I would recommend you use v2 so you get type safety.