TreeList/CursorableLinkedList/NodeCachingLinkedList/AbstractLinkedList - Fix iterator remove not working properly when called after previous

bug 35258

git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/collections/trunk@219343 13f79535-47bb-0310-9956-ffa450edef68

RELEASE-NOTES.html

@@ -75,6 +75,7 @@ If this causes major headaches to anyone please contact commons-dev at jakarta.a
 <li>FastArrayList - Fix iterators and views to work better in multithreaded environments</li>
 <li>FastArrayList - Fix iterator remove where ConcurrentModificationException not as expected [34690]</li>
 <li>CursorableLinkedList (list subpackage) - Fix iterator remove/set not throwing IllegalStateException after next-previous-removeByIndex [35766]</li>
+<li>TreeList/CursorableLinkedList/NodeCachingLinkedList/AbstractLinkedList - Fix iterator remove not working properly when called after previous [35258]</li>
 <li>SetUniqueList.set(int,Object) - Destroyed set status in certain circumstances [33294]</li>
 <li>AbstractLinkedMap.init() - Now calls createEntry() to create the map entry object [33706]</li>
 <li>AbstractHashedMap deserialization - Fix to prevent doubling of internal data array [34265]</li>

src/java/org/apache/commons/collections/list/AbstractLinkedList.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright 2001-2004 The Apache Software Foundation
+ *  Copyright 2001-2005 The Apache Software Foundation
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -819,9 +819,16 @@ public abstract class AbstractLinkedList implements List {
 
         public void remove() {
             checkModCount();
-            parent.removeNode(getLastNodeReturned());
+            if (current == next) {
+                // remove() following previous()
+                next = next.next;
+                parent.removeNode(getLastNodeReturned());
+            } else {
+                // remove() following next()
+                parent.removeNode(getLastNodeReturned());
+                nextIndex--;
+            }
             current = null;
-            nextIndex--;
             expectedModCount++;
         }
 
@@ -885,13 +892,13 @@ public abstract class AbstractLinkedList implements List {
      */
     protected static class LinkedSubList extends AbstractList {
         /** The main list */
-        private AbstractLinkedList parent;
+        AbstractLinkedList parent;
         /** Offset from the main list */
-        private int offset;
+        int offset;
         /** Sublist size */
-        private int size;
+        int size;
         /** Sublist modCount */
-        private int expectedModCount;
+        int expectedModCount;
 
         protected LinkedSubList(AbstractLinkedList parent, int fromIndex, int toIndex) {
             if (fromIndex < 0) {

src/java/org/apache/commons/collections/list/CursorableLinkedList.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright 2002-2004 The Apache Software Foundation
+ *  Copyright 2002-2005 The Apache Software Foundation
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -40,7 +40,7 @@ import java.util.ListIterator;
  * methods provides access to a <code>Cursor</code> instance which extends
  * <code>ListIterator</code>. The cursor allows changes to the list concurrent
  * with changes to the iterator. Note that the {@link #iterator()} method and
- * sublists  do <b>not</b> provide this cursor behaviour.
+ * sublists do <b>not</b> provide this cursor behaviour.
  * <p>
  * The <code>Cursor</code> class is provided partly for backwards compatibility
  * and partly because it allows the cursor to be directly closed. Closing the
@@ -374,6 +374,19 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
         doReadObject(in);
     }
 
+    //-----------------------------------------------------------------------
+    /**
+     * Creates a list iterator for the sublist.
+     * 
+     * @param subList  the sublist to get an iterator for
+     * @param fromIndex  the index to start from, relative to the sublist
+     */
+    protected ListIterator createSubListListIterator(LinkedSubList subList, int fromIndex) {
+        SubCursor cursor = new SubCursor(subList, fromIndex);
+        registerCursor(cursor);
+        return cursor;
+    }
+
     //-----------------------------------------------------------------------
     /**
      * An extended <code>ListIterator</code> that allows concurrent changes to
@@ -384,6 +397,8 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
         boolean valid = true;
         /** Is the next index valid */
         boolean nextIndexValid = true;
+        /** Flag to indicate if the current element was removed by another object. */
+        boolean currentRemovedByAnother = false;
         
         /**
          * Constructs a new cursor.
@@ -394,7 +409,33 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
             super(parent, index);
             valid = true;
         }
-        
+
+        /**
+         * Removes the item last returned by this iterator.
+         * <p>
+         * There may have been subsequent alterations to the list
+         * since you obtained this item, however you can still remove it.
+         * You can even remove it if the item is no longer in the main list.
+         * However, you can't call this method on the same iterator more
+         * than once without calling next() or previous().
+         *
+         * @throws IllegalStateException if there is no item to remove
+         */
+        public void remove() {
+            // overridden, as the nodeRemoved() method updates the iterator
+            // state in the parent.removeNode() call below
+            if (current == null && currentRemovedByAnother) {
+                // quietly ignore, as the last returned node was removed
+                // by the list or some other iterator
+                // by ignoring it, we keep this iterator independent from
+                // other changes as much as possible
+            } else {
+                checkModCount();
+                parent.removeNode(getLastNodeReturned());
+            }
+            currentRemovedByAnother = false;
+        }
+
         /**
          * Adds an object to the list.
          * The object added here will be the new 'previous' in the iterator.
@@ -402,10 +443,17 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
          * @param obj  the object to add
          */
         public void add(Object obj) {
+            // overridden, as the nodeInserted() method updates the iterator state
             super.add(obj);
-            // add on iterator does not return the added element
+            // matches the (next.previous == node) clause in nodeInserted()
+            // thus next gets changed - reset it again here
             next = next.next;
         }
+        
+        // set is not overridden, as it works ok
+        // note that we want it to throw an exception if the element being
+        // set has been removed from the real list (compare this with the
+        // remove method where we silently ignore this case)
 
         /**
          * Gets the index of the next element to be returned.
@@ -449,17 +497,21 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
                 // state where next() followed by previous()
                 next = node.next;
                 current = null;
+                currentRemovedByAnother = true;
             } else if (node == next) {
                 // state where next() not followed by previous()
                 // and we are matching next node
                 next = node.next;
+                currentRemovedByAnother = false;
             } else if (node == current) {
                 // state where next() not followed by previous()
                 // and we are matching current (last returned) node
                 current = null;
+                currentRemovedByAnother = true;
                 nextIndex--;
             } else {
                 nextIndexValid = false;
+                currentRemovedByAnother = false;
             }
         }
 
@@ -502,4 +554,49 @@ public class CursorableLinkedList extends AbstractLinkedList implements Serializ
             }
         }
     }
+
+    //-----------------------------------------------------------------------
+    /**
+     * A cursor for the sublist based on LinkedSubListIterator.
+     */
+    protected static class SubCursor extends Cursor {
+
+        /** The parent list */
+        protected final LinkedSubList sub;
+
+        /**
+         * Constructs a new cursor.
+         * 
+         * @param index  the index to start from
+         */
+        protected SubCursor(LinkedSubList sub, int index) {
+            super((CursorableLinkedList) sub.parent, index + sub.offset);
+            this.sub = sub;
+        }
+
+        public boolean hasNext() {
+            return (nextIndex() < sub.size);
+        }
+
+        public boolean hasPrevious() {
+            return (previousIndex() >= 0);
+        }
+
+        public int nextIndex() {
+            return (super.nextIndex() - sub.offset);
+        }
+
+        public void add(Object obj) {
+            super.add(obj);
+            sub.expectedModCount = parent.modCount;
+            sub.size++;
+        }
+
+        public void remove() {
+            super.remove();
+            sub.expectedModCount = parent.modCount;
+            sub.size--;
+        }
+    }
+
 }

src/java/org/apache/commons/collections/list/TreeList.java

@@ -749,23 +749,20 @@ public class TreeList extends AbstractList {
         /** The parent list */
         protected final TreeList parent;
         /**
-         * The node that will be returned by {@link #next()}. If this is equal
+         * Cache of the next node that will be returned by {@link #next()}.
-         * to {@link AbstractLinkedList#header} then there are no more values to return.
          */
         protected AVLNode next;
         /**
-         * The index of {@link #next}.
+         * The index of the next node to be returned.
          */
         protected int nextIndex;
         /**
-         * The last node that was returned by {@link #next()} or {@link
+         * Cache of the last node that was returned by {@link #next()}
-         * #previous()}. Set to <code>null</code> if {@link #next()} or {@link
+         * or {@link #previous()}.
-         * #previous()} haven't been called, or if the node has been removed
-         * with {@link #remove()} or a new node added with {@link #add(Object)}.
          */
         protected AVLNode current;
         /**
-         * The index of {@link #current}.
+         * The index of the last node that was returned.
          */
         protected int currentIndex;
         /**
@@ -788,6 +785,7 @@ public class TreeList extends AbstractList {
             this.expectedModCount = parent.modCount;
             this.next = (parent.root == null ? null : parent.root.get(fromIndex));
             this.nextIndex = fromIndex;
+            this.currentIndex = -1;
         }
 
         /**
@@ -852,13 +850,20 @@ public class TreeList extends AbstractList {
 
         public void remove() {
             checkModCount();
-            if (current == null) {
+            if (currentIndex == -1) {
                 throw new IllegalStateException();
             }
-            parent.remove(currentIndex);
+            if (nextIndex == currentIndex) {
+                // remove() following previous()
+                next = next.next();
+                parent.remove(currentIndex);
+            } else {
+                // remove() following next()
+                parent.remove(currentIndex);
+                nextIndex--;
+            }
             current = null;
             currentIndex = -1;
-            nextIndex--;
             expectedModCount++;
         }
 

src/test/org/apache/commons/collections/list/AbstractTestList.java

@@ -1,5 +1,5 @@
 /*
- *  Copyright 2001-2004 The Apache Software Foundation
+ *  Copyright 2001-2005 The Apache Software Foundation
  *
  *  Licensed under the Apache License, Version 2.0 (the "License");
  *  you may not use this file except in compliance with the License.
@@ -796,10 +796,11 @@ public abstract class AbstractTestList extends AbstractTestCollection {
         }
     }
 
+    //-----------------------------------------------------------------------
     /**
      * Tests remove on list iterator is correct.
      */
-    public void testListListIteratorPreviousRemove() {
+    public void testListListIteratorPreviousRemoveNext() {
         if (isRemoveSupported() == false) return;
         resetFull();
         ListIterator it = getList().listIterator();
@@ -813,11 +814,91 @@ public abstract class AbstractTestList extends AbstractTestCollection {
         assertSame(zero, getList().get(0));
         assertSame(one, getList().get(1));
         assertSame(two, getList().get(2));
-        it.remove();
+        
+        it.remove(); // removed element at index 1 (one)
         assertSame(zero, getList().get(0));
         assertSame(two, getList().get(1));
+        Object two3 = it.next();  // do next after remove
+        assertSame(two, two3);
+        assertEquals(collection.size() > 2, it.hasNext());
+        assertEquals(true, it.hasPrevious());
     }
 
+    /**
+     * Tests remove on list iterator is correct.
+     */
+    public void testListListIteratorPreviousRemovePrevious() {
+        if (isRemoveSupported() == false) return;
+        resetFull();
+        ListIterator it = getList().listIterator();
+        Object zero = it.next();
+        Object one = it.next();
+        Object two = it.next();
+        Object two2 = it.previous();
+        Object one2 = it.previous();
+        assertSame(one, one2);
+        assertSame(two, two2);
+        assertSame(zero, getList().get(0));
+        assertSame(one, getList().get(1));
+        assertSame(two, getList().get(2));
+        
+        it.remove(); // removed element at index 1 (one)
+        assertSame(zero, getList().get(0));
+        assertSame(two, getList().get(1));
+        Object zero3 = it.previous();  // do previous after remove
+        assertSame(zero, zero3);
+        assertEquals(false, it.hasPrevious());
+        assertEquals(collection.size() > 2, it.hasNext());
+    }
+
+    /**
+     * Tests remove on list iterator is correct.
+     */
+    public void testListListIteratorNextRemoveNext() {
+        if (isRemoveSupported() == false) return;
+        resetFull();
+        ListIterator it = getList().listIterator();
+        Object zero = it.next();
+        Object one = it.next();
+        Object two = it.next();
+        assertSame(zero, getList().get(0));
+        assertSame(one, getList().get(1));
+        assertSame(two, getList().get(2));
+        Object three = getList().get(3);
+        
+        it.remove(); // removed element at index 2 (two)
+        assertSame(zero, getList().get(0));
+        assertSame(one, getList().get(1));
+        Object three2 = it.next();  // do next after remove
+        assertSame(three, three2);
+        assertEquals(collection.size() > 3, it.hasNext());
+        assertEquals(true, it.hasPrevious());
+    }
+
+    /**
+     * Tests remove on list iterator is correct.
+     */
+    public void testListListIteratorNextRemovePrevious() {
+        if (isRemoveSupported() == false) return;
+        resetFull();
+        ListIterator it = getList().listIterator();
+        Object zero = it.next();
+        Object one = it.next();
+        Object two = it.next();
+        assertSame(zero, getList().get(0));
+        assertSame(one, getList().get(1));
+        assertSame(two, getList().get(2));
+        
+        it.remove(); // removed element at index 2 (two)
+        assertSame(zero, getList().get(0));
+        assertSame(one, getList().get(1));
+        Object one2 = it.previous();  // do previous after remove
+        assertSame(one, one2);
+        assertEquals(true, it.hasNext());
+        assertEquals(true, it.hasPrevious());
+    }
+
+    //-----------------------------------------------------------------------
     /**
      *  Traverses to the end of the given iterator.
      *

src/test/org/apache/commons/collections/list/TestCursorableLinkedList.java

@@ -460,8 +460,17 @@ public class TestCursorableLinkedList extends TestAbstractLinkedList {
         
         assertEquals(true, c1.nextIndexValid);
         assertEquals(1, c1.nextIndex);
+        assertEquals(true, c1.currentRemovedByAnother);
         assertEquals(null, c1.current);
         assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
     }
 
     public void testInternalState_CursorNextRemoveIndex1ByList() {
@@ -476,8 +485,17 @@ public class TestCursorableLinkedList extends TestAbstractLinkedList {
         
         assertEquals(true, c1.nextIndexValid);
         assertEquals(1, c1.nextIndex);
+        assertEquals(false, c1.currentRemovedByAnother);
         assertEquals("A", c1.current.value);
         assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
     }
 
     public void testInternalState_CursorNextNextRemoveIndex1ByList() {
@@ -493,8 +511,17 @@ public class TestCursorableLinkedList extends TestAbstractLinkedList {
         
         assertEquals(true, c1.nextIndexValid);
         assertEquals(1, c1.nextIndex);
+        assertEquals(true, c1.currentRemovedByAnother);
         assertEquals(null, c1.current);
         assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
     }
 
     public void testInternalState_CursorNextNextNextRemoveIndex1ByList() {
@@ -511,8 +538,267 @@ public class TestCursorableLinkedList extends TestAbstractLinkedList {
         assertEquals("B", list.remove(1));
         
         assertEquals(false, c1.nextIndexValid);
+        assertEquals(false, c1.currentRemovedByAnother);
         assertEquals("C", c1.current.value);
         assertEquals("D", c1.next.value);
+        
+        assertEquals("[A, C, D]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, D]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    //-----------------------------------------------------------------------
+    public void testInternalState_CursorNextNextPreviousRemoveByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        assertEquals("B", c1.previous());
+        
+        c1.remove();
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals(false, c1.currentRemovedByAnother);
+        assertEquals(null, c1.current);
+        assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    public void testInternalState_CursorNextNextRemoveByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        
+        c1.remove();
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals(false, c1.currentRemovedByAnother);
+        assertEquals(null, c1.current);
+        assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    //-----------------------------------------------------------------------
+    public void testInternalState_CursorNextNextPreviousAddIndex1ByList() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        assertEquals("B", c1.previous());
+        
+        list.add(1, "Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals("B", c1.current.value);
+        assertEquals("Z", c1.next.value);
+        
+        assertEquals("[A, Z, B, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, Z, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    public void testInternalState_CursorNextAddIndex1ByList() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        
+        list.add(1, "Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals("A", c1.current.value);
+        assertEquals("Z", c1.next.value);
+        
+        assertEquals("[A, Z, B, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[Z, B, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    public void testInternalState_CursorNextNextAddIndex1ByList() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        
+        list.add(1, "Z");
+        
+        assertEquals(false, c1.nextIndexValid);
+        assertEquals("B", c1.current.value);
+        assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, Z, B, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, Z, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    //-----------------------------------------------------------------------
+    public void testInternalState_CursorNextNextPreviousAddByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        assertEquals("B", c1.previous());
+        
+        c1.add("Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(2, c1.nextIndex);
+        assertEquals(null, c1.current);
+        assertEquals("B", c1.next.value);
+        
+        assertEquals("[A, Z, B, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    public void testInternalState_CursorNextNextAddByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        
+        c1.add("Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(3, c1.nextIndex);
+        assertEquals(false, c1.currentRemovedByAnother);
+        assertEquals(null, c1.current);
+        assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, B, Z, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    //-----------------------------------------------------------------------
+    public void testInternalState_CursorNextNextRemoveByListSetByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        
+        list.remove(1);
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals(null, c1.current);
+        assertEquals("C", c1.next.value);
+        assertEquals("[A, C]", list.toString());
+        
+        try {
+            c1.set("Z");
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    //-----------------------------------------------------------------------
+    public void testInternalState_CursorNextNextPreviousSetByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        assertEquals("B", c1.previous());
+        
+        c1.set("Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(1, c1.nextIndex);
+        assertEquals("Z", c1.current.value);
+        assertEquals("Z", c1.next.value);
+        
+        assertEquals("[A, Z, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
+    }
+
+    public void testInternalState_CursorNextNextSetByIterator() {
+        list.add("A");
+        list.add("B");
+        list.add("C");
+
+        CursorableLinkedList.Cursor c1 = list.cursor();
+        assertEquals("A", c1.next());
+        assertEquals("B", c1.next());
+        
+        c1.set("Z");
+        
+        assertEquals(true, c1.nextIndexValid);
+        assertEquals(2, c1.nextIndex);
+        assertEquals("Z", c1.current.value);
+        assertEquals("C", c1.next.value);
+        
+        assertEquals("[A, Z, C]", list.toString());
+        c1.remove();  // works ok
+        assertEquals("[A, C]", list.toString());
+        try {
+            c1.remove();
+            fail();
+        } catch (IllegalStateException ex) {}
     }
 
     //-----------------------------------------------------------------------