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.