]> Cypherpunks repositories - gostls13.git/commitdiff
container/list: Correctly maintain internal invariants
authorRobert Griesemer <gri@golang.org>
Fri, 28 Sep 2012 17:35:32 +0000 (10:35 -0700)
committerRobert Griesemer <gri@golang.org>
Fri, 28 Sep 2012 17:35:32 +0000 (10:35 -0700)
The previous implementation was a mess with invariants
maintained inconsistently. Essentially reimplemented
the package:

- used a circular list as internal representation for
  significantly simpler implementation with fewer
  special cases while maintaining the illusion of
  a nil-terminated doubly linked list externally

- more precise documentation

- cleaned up and simplified tests, added test case
  for issue 4103.

No changes to the API or documented semantics.

All this said, I would be in favor of removing
this package eventually. container/ring provides
a faster implementation and a simpler and more
powerful API.

Fixes #4103.

R=r
CC=golang-dev
https://golang.org/cl/6569072

src/pkg/container/list/list.go
src/pkg/container/list/list_test.go

index a3fd4b39f322433fa05fc276309aecf558e18440..17f5d17e695412dae2b3f3148434c26834c3936e 100644 (file)
 //
 package list
 
-// Element is an element in the linked list.
+// Element is an element of a linked list.
 type Element struct {
        // Next and previous pointers in the doubly-linked list of elements.
-       // The front of the list has prev = nil, and the back has next = nil.
+       // To simplify the implementation, internally a list l is implemented
+       // as a ring, such that &l.root is both the next element of the last
+       // list element (l.Back()) and the previous element of the first list
+       // element (l.Front()).
        next, prev *Element
 
        // The list to which this element belongs.
        list *List
 
-       // The contents of this list element.
+       // The value stored with this element.
        Value interface{}
 }
 
 // Next returns the next list element or nil.
-func (e *Element) Next() *Element { return e.next }
+func (e *Element) Next() *Element {
+       if p := e.next; p != &e.list.root {
+               return p
+       }
+       return nil
+}
 
 // Prev returns the previous list element or nil.
-func (e *Element) Prev() *Element { return e.prev }
+func (e *Element) Prev() *Element {
+       if p := e.prev; p != &e.list.root {
+               return p
+       }
+       return nil
+}
 
 // List represents a doubly linked list.
 // The zero value for List is an empty list ready to use.
 type List struct {
-       front, back *Element
-       len         int
+       root Element // sentinel list element, only &root, root.prev, and root.next are used
+       len  int     // current list length excluding (this) sentinel element
 }
 
-// Init initializes or clears a List.
+// Init initializes or clears list l.
 func (l *List) Init() *List {
-       l.front = nil
-       l.back = nil
+       l.root.next = &l.root
+       l.root.prev = &l.root
        l.len = 0
        return l
 }
 
 // New returns an initialized list.
-func New() *List { return new(List) }
+func New() *List { return new(List).Init() }
 
-// Front returns the first element in the list.
-func (l *List) Front() *Element { return l.front }
-
-// Back returns the last element in the list.
-func (l *List) Back() *Element { return l.back }
+// Len returns the number of elements of list l.
+func (l *List) Len() int { return l.len }
 
-// Remove removes the element from the list
-// and returns its Value.
-func (l *List) Remove(e *Element) interface{} {
-       l.remove(e)
-       e.list = nil // do what remove does not
-       return e.Value
+// Front returns the first element of list l or nil
+func (l *List) Front() *Element {
+       if l.len == 0 {
+               return nil
+       }
+       return l.root.next
 }
 
-// remove the element from the list, but do not clear the Element's list field.
-// This is so that other List methods may use remove when relocating Elements
-// without needing to restore the list field.
-func (l *List) remove(e *Element) {
-       if e.list != l {
-               return
-       }
-       if e.prev == nil {
-               l.front = e.next
-       } else {
-               e.prev.next = e.next
-       }
-       if e.next == nil {
-               l.back = e.prev
-       } else {
-               e.next.prev = e.prev
+// Back returns the last element of list l or nil.
+func (l *List) Back() *Element {
+       if l.len == 0 {
+               return nil
        }
-
-       e.prev = nil
-       e.next = nil
-       l.len--
+       return l.root.prev
 }
 
-func (l *List) insertBefore(e *Element, mark *Element) {
-       if mark.prev == nil {
-               // new front of the list
-               l.front = e
-       } else {
-               mark.prev.next = e
+// lazyInit lazily initializes a zero List value.
+func (l *List) lazyInit() {
+       if l.root.next == nil {
+               l.root.next = &l.root
+               l.root.prev = &l.root
        }
-       e.prev = mark.prev
-       mark.prev = e
-       e.next = mark
-       l.len++
 }
 
-func (l *List) insertAfter(e *Element, mark *Element) {
-       if mark.next == nil {
-               // new back of the list
-               l.back = e
-       } else {
-               mark.next.prev = e
-       }
-       e.next = mark.next
-       mark.next = e
-       e.prev = mark
+// insert inserts e after at, increments l.len, and returns e.
+func (l *List) insert(e, at *Element) *Element {
+       n := at.next
+       at.next = e
+       e.prev = at
+       e.next = n
+       n.prev = e
+       e.list = l
        l.len++
+       return e
 }
 
-func (l *List) insertFront(e *Element) {
-       if l.front == nil {
-               // empty list
-               l.front, l.back = e, e
-               e.prev, e.next = nil, nil
-               l.len = 1
-               return
-       }
-       l.insertBefore(e, l.front)
+// remove removes e from its list, decrements l.len, and returns e.
+func (l *List) remove(e *Element) *Element {
+       e.prev.next = e.next
+       e.next.prev = e.prev
+       e.list = nil
+       l.len--
+       return e
 }
 
-func (l *List) insertBack(e *Element) {
-       if l.back == nil {
-               // empty list
-               l.front, l.back = e, e
-               e.prev, e.next = nil, nil
-               l.len = 1
-               return
+// Remove removes e from l if e is an element of list l.
+// It returns the element value e.Value.
+func (l *List) Remove(e *Element) interface{} {
+       if e.list == l {
+               // if e.list == l, l must have been initialized when e was inserted
+               // in l or l == nil (e is a zero Element) and l.remove will crash
+               l.remove(e)
        }
-       l.insertAfter(e, l.back)
+       return e.Value
 }
 
-// PushFront inserts the value at the front of the list and returns a new Element containing the value.
-func (l *List) PushFront(value interface{}) *Element {
-       e := &Element{nil, nil, l, value}
-       l.insertFront(e)
-       return e
+// Pushfront inserts a new element e with value v at the front of list l and returns e.
+func (l *List) PushFront(v interface{}) *Element {
+       l.lazyInit()
+       return l.insert(&Element{Value: v}, &l.root)
 }
 
-// PushBack inserts the value at the back of the list and returns a new Element containing the value.
-func (l *List) PushBack(value interface{}) *Element {
-       e := &Element{nil, nil, l, value}
-       l.insertBack(e)
-       return e
+// PushBack inserts a new element e with value v at the back of list l and returns e.
+func (l *List) PushBack(v interface{}) *Element {
+       l.lazyInit()
+       return l.insert(&Element{Value: v}, l.root.prev)
 }
 
-// InsertBefore inserts the value immediately before mark and returns a new Element containing the value.
-func (l *List) InsertBefore(value interface{}, mark *Element) *Element {
+// InsertBefore inserts a new element e with value v immediately before mark and returns e.
+// If mark is not an element of l, the list is not modified.
+func (l *List) InsertBefore(v interface{}, mark *Element) *Element {
        if mark.list != l {
                return nil
        }
-       e := &Element{nil, nil, l, value}
-       l.insertBefore(e, mark)
-       return e
+       // see comment in List.Remove about initialization of l
+       return l.insert(&Element{Value: v}, mark.prev)
 }
 
-// InsertAfter inserts the value immediately after mark and returns a new Element containing the value.
+// InsertAfter inserts a new element e with value v immediately after mark and returns e.
+// If mark is not an element of l, the list is not modified.
 func (l *List) InsertAfter(value interface{}, mark *Element) *Element {
        if mark.list != l {
                return nil
        }
-       e := &Element{nil, nil, l, value}
-       l.insertAfter(e, mark)
-       return e
+       // see comment in List.Remove about initialization of l
+       return l.insert(&Element{Value: value}, mark)
 }
 
-// MoveToFront moves the element to the front of the list.
+// MoveToFront moves element e to the front of list l.
+// If e is not an element of l, the list is not modified.
 func (l *List) MoveToFront(e *Element) {
-       if e.list != l || l.front == e {
+       if e.list != l || l.root.next == e {
                return
        }
-       l.remove(e)
-       l.insertFront(e)
+       // see comment in List.Remove about initialization of l
+       l.insert(l.remove(e), &l.root)
 }
 
-// MoveToBack moves the element to the back of the list.
+// MoveToBack moves element e to the back of list l.
+// If e is not an element of l, the list is not modified.
 func (l *List) MoveToBack(e *Element) {
-       if e.list != l || l.back == e {
+       if e.list != l || l.root.prev == e {
                return
        }
-       l.remove(e)
-       l.insertBack(e)
+       // see comment in List.Remove about initialization of l
+       l.insert(l.remove(e), l.root.prev)
 }
 
-// Len returns the number of elements in the list.
-func (l *List) Len() int { return l.len }
-
-// PushBackList inserts each element of ol at the back of the list.
-func (l *List) PushBackList(ol *List) {
-       last := ol.Back()
-       for e := ol.Front(); e != nil; e = e.Next() {
-               l.PushBack(e.Value)
-               if e == last {
-                       break
-               }
+// PuchBackList inserts a copy of an other list at the back of list l.
+// The lists l and other may be the same.
+func (l *List) PushBackList(other *List) {
+       l.lazyInit()
+       for i, e := other.Len(), other.Front(); i > 0; i, e = i-1, e.Next() {
+               l.insert(&Element{Value: e.Value}, l.root.prev)
        }
 }
 
-// PushFrontList inserts each element of ol at the front of the list. The ordering of the passed list is preserved.
-func (l *List) PushFrontList(ol *List) {
-       first := ol.Front()
-       for e := ol.Back(); e != nil; e = e.Prev() {
-               l.PushFront(e.Value)
-               if e == first {
-                       break
-               }
+// PushFrontList inserts a copy of an other list at the front of list l.
+// The lists l and other may be the same.
+func (l *List) PushFrontList(other *List) {
+       l.lazyInit()
+       for i, e := other.Len(), other.Back(); i > 0; i, e = i-1, e.Prev() {
+               l.insert(&Element{Value: e.Value}, &l.root)
        }
 }
index 1d44ff84e4a477b40358e1488e35cdb74b7f61b1..b4fc77d1403546e732e46ec79062c6e6f8221604 100644 (file)
@@ -4,65 +4,75 @@
 
 package list
 
-import (
-       "testing"
-)
+import "testing"
+
+func checkListLen(t *testing.T, l *List, len int) bool {
+       if n := l.Len(); n != len {
+               t.Errorf("l.Len() = %d, want %d", n, len)
+               return false
+       }
+       return true
+}
 
 func checkListPointers(t *testing.T, l *List, es []*Element) {
-       if len(es) == 0 {
-               if l.front != nil || l.back != nil {
-                       t.Errorf("l.front/l.back = %v/%v should be nil/nil", l.front, l.back)
-               }
+       root := &l.root
+
+       if !checkListLen(t, l, len(es)) {
                return
        }
 
-       if l.front != es[0] {
-               t.Errorf("l.front = %v, want %v", l.front, es[0])
-       }
-       if last := es[len(es)-1]; l.back != last {
-               t.Errorf("l.back = %v, want %v", l.back, last)
+       // zero length lists must be the zero value or properly initialized (sentinel circle)
+       if len(es) == 0 {
+               if l.root.next != nil && l.root.next != root || l.root.prev != nil && l.root.prev != root {
+                       t.Errorf("l.root.next = %p, l.root.prev = %p; both should both be nil or %p", l.root.next, l.root.prev, root)
+               }
+               return
        }
+       // len(es) > 0
 
+       // check internal and external prev/next connections
        for i, e := range es {
-               var e_prev, e_next *Element = nil, nil
+               prev := root
+               Prev := (*Element)(nil)
                if i > 0 {
-                       e_prev = es[i-1]
+                       prev = es[i-1]
+                       Prev = prev
+               }
+               if p := e.prev; p != prev {
+                       t.Errorf("elt[%d](%p).prev = %p, want %p", i, e, p, prev)
+               }
+               if p := e.Prev(); p != Prev {
+                       t.Errorf("elt[%d](%p).Prev() = %p, want %p", i, e, p, Prev)
                }
+
+               next := root
+               Next := (*Element)(nil)
                if i < len(es)-1 {
-                       e_next = es[i+1]
+                       next = es[i+1]
+                       Next = next
                }
-               if e.prev != e_prev {
-                       t.Errorf("elt #%d (%v) has prev=%v, want %v", i, e, e.prev, e_prev)
+               if n := e.next; n != next {
+                       t.Errorf("elt[%d](%p).next = %p, want %p", i, e, n, next)
                }
-               if e.next != e_next {
-                       t.Errorf("elt #%d (%v) has next=%v, want %v", i, e, e.next, e_next)
+               if n := e.Next(); n != Next {
+                       t.Errorf("elt[%d](%p).Next() = %p, want %p", i, e, n, Next)
                }
        }
 }
 
-func checkListLen(t *testing.T, l *List, n int) {
-       if an := l.Len(); an != n {
-               t.Errorf("l.Len() = %d, want %d", an, n)
-       }
-}
-
 func TestList(t *testing.T) {
        l := New()
        checkListPointers(t, l, []*Element{})
-       checkListLen(t, l, 0)
 
        // Single element list
        e := l.PushFront("a")
-       checkListLen(t, l, 1)
        checkListPointers(t, l, []*Element{e})
        l.MoveToFront(e)
        checkListPointers(t, l, []*Element{e})
        l.MoveToBack(e)
        checkListPointers(t, l, []*Element{e})
-       checkListLen(t, l, 1)
        l.Remove(e)
        checkListPointers(t, l, []*Element{})
-       checkListLen(t, l, 0)
 
        // Bigger list
        e2 := l.PushFront(2)
@@ -70,11 +80,9 @@ func TestList(t *testing.T) {
        e3 := l.PushBack(3)
        e4 := l.PushBack("banana")
        checkListPointers(t, l, []*Element{e1, e2, e3, e4})
-       checkListLen(t, l, 4)
 
        l.Remove(e2)
        checkListPointers(t, l, []*Element{e1, e3, e4})
-       checkListLen(t, l, 3)
 
        l.MoveToFront(e3) // move from middle
        checkListPointers(t, l, []*Element{e3, e1, e4})
@@ -121,7 +129,7 @@ func TestList(t *testing.T) {
                }
        }
        if sum != 4 {
-               t.Errorf("sum over l.Iter() = %d, want 4", sum)
+               t.Errorf("sum over l = %d, want 4", sum)
        }
 
        // Clear all elements by iterating
@@ -131,19 +139,18 @@ func TestList(t *testing.T) {
                l.Remove(e)
        }
        checkListPointers(t, l, []*Element{})
-       checkListLen(t, l, 0)
 }
 
 func checkList(t *testing.T, l *List, es []interface{}) {
-       if l.Len() != len(es) {
-               t.Errorf("list has len=%v, want %v", l.Len(), len(es))
+       if !checkListLen(t, l, len(es)) {
                return
        }
+
        i := 0
        for e := l.Front(); e != nil; e = e.Next() {
                le := e.Value.(int)
                if le != es[i] {
-                       t.Errorf("elt #%d has value=%v, want %v", i, le, es[i])
+                       t.Errorf("elt[%d].Value = %v, want %v", i, le, es[i])
                }
                i++
        }
@@ -202,8 +209,27 @@ func TestRemove(t *testing.T) {
        e := l.Front()
        l.Remove(e)
        checkListPointers(t, l, []*Element{e2})
-       checkListLen(t, l, 1)
        l.Remove(e)
        checkListPointers(t, l, []*Element{e2})
-       checkListLen(t, l, 1)
+}
+
+func TestIssue4103(t *testing.T) {
+       l1 := New()
+       l1.PushBack(1)
+       l1.PushBack(2)
+
+       l2 := New()
+       l2.PushBack(3)
+       l2.PushBack(4)
+
+       e := l1.Front()
+       l2.Remove(e) // l2 should not change because e is not an element of l2
+       if n := l2.Len(); n != 2 {
+               t.Errorf("l2.Len() = %d, want 2", n)
+       }
+
+       l1.InsertBefore(8, e)
+       if n := l1.Len(); n != 3 {
+               t.Errorf("l1.Len() = %d, want 3", n)
+       }
 }