Fixed inconsistent behavior and added missing impls in iterator of ar…

…ray-based containers
vigna · Jun 14, 2024 · defd2d1 · defd2d1
1 parent a439dee
commit defd2d1
Show file tree

Hide file tree

Showing 8 changed files with 219 additions and 11 deletions.
diff --git a/CHANGES b/CHANGES
@@ -5,6 +5,13 @@
 - Fixed a bug in sublist iterators of immutable lists. Thanks to Barak
   Ugav for finding and fixing this bug.
 
+- Implemented missing skip() and forEachRemaining() methods in array-based
+  containers.
+
+- Fixed a bug in array-based containers that would have thrown the wrong
+  exception and leave the iterator in an inconsistent state when removing
+  before iterating. Thanks to Michal Frajt for reporting this bug.
+
 8.5.13
 
 - Thanks to Chanoch Goldfeder for fixing a number of bugs in ImmutableList.

diff --git a/drv/ArrayMap.drv b/drv/ArrayMap.drv
@@ -174,6 +174,15 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
 #endif
 				}
 
+				@Override
+				public int skip(int n) {
+					if (n < 0) throw new IllegalArgumentException("Argument must be nonnegative: " + n);
+					n = Math.min(n, size - next);
+					next += n;
+					if (n != 0) curr = next - 1;
+					return n;
+				}
+
 				@Override
 				SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
 				public void forEachRemaining(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {
@@ -220,6 +229,16 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
 #endif
 				}
 
+				@Override
+				public int skip(int n) {
+					if (n < 0) throw new IllegalArgumentException("Argument must be nonnegative: " + n);
+					n = Math.min(n, size - next);
+					next += n;
+					if (n != 0) curr = next - 1;
+					return n;
+				}
+
+
 				@Override
 				SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
 				public void forEachRemaining(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {

diff --git a/drv/ArraySet.drv b/drv/ArraySet.drv
@@ -194,7 +194,7 @@ public class ARRAY_SET KEY_GENERIC extends ABSTRACT_SET KEY_GENERIC implements j
 	SUPPRESS_WARNINGS_KEY_UNCHECKED
 	public KEY_ITERATOR KEY_GENERIC iterator() {
 		return new KEY_ITERATOR KEY_GENERIC () {
-			int next = 0;
+			int curr = -1, next = 0;
 
 			@Override
 			public boolean hasNext() { return next < size; }
@@ -207,6 +207,8 @@ public class ARRAY_SET KEY_GENERIC extends ABSTRACT_SET KEY_GENERIC implements j
 
 			@Override
 			public void remove() {
+				if (curr == -1) throw new IllegalStateException();
+				curr = -1;
 				final int tail = size-- - next--;
 				System.arraycopy(a, next + 1, a, next, tail);
 #if KEYS_REFERENCE
@@ -217,15 +219,17 @@ public class ARRAY_SET KEY_GENERIC extends ABSTRACT_SET KEY_GENERIC implements j
 			@Override
 			public int skip(int n) {
 				if (n < 0) throw new IllegalArgumentException("Argument must be nonnegative: " + n);
-				final int remaining = size - next;
-				if (n < remaining) {
-					next += n;
-					return n;
-				}
-				n = remaining;
-				next = size;
+				n = Math.min(n, size - next);
+				next += n;
+				if (n != 0) curr = next - 1;
 				return n;
 			}
+
+			@Override
+			public void forEachRemaining(final METHOD_ARG_KEY_CONSUMER action) {
+				final KEY_TYPE a[] = ARRAY_SET.this.a;
+				while (next < size) action.accept(KEY_GENERIC_CAST a[next++]);
+			}
 		};
 	}
 

diff --git a/guava/it/unimi/dsi/fastutil/ints/IntSetGuavaTest.java b/guava/it/unimi/dsi/fastutil/ints/IntSetGuavaTest.java
@@ -72,4 +72,5 @@ public SampleElements<Integer> samples() {
 			}
 		}).named(name).withFeatures(CollectionSize.ANY, SetFeature.GENERAL_PURPOSE, CollectionFeature.ALLOWS_NULL_QUERIES, CollectionFeature.RESTRICTS_ELEMENTS, CollectionFeature.SUPPORTS_ADD, CollectionFeature.SUPPORTS_REMOVE, CollectionFeature.SERIALIZABLE, CollectionFeature.REMOVE_OPERATIONS, CollectionFeature.SUPPORTS_ITERATOR_REMOVE).suppressing(toStringTests).createTestSuite();
 	}
+
 }
diff --git a/test/it/unimi/dsi/fastutil/ints/Int2IntArrayMapTest.java b/test/it/unimi/dsi/fastutil/ints/Int2IntArrayMapTest.java
@@ -52,4 +52,48 @@ public void testValuesRemoveAll() {
 		map.values().removeAll(Collections.singleton(Integer.valueOf(24)));
 		assertTrue(map.isEmpty());
 	}
+
+	@Test
+	public void testForEachRemaining() {
+		final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1, 2, 3,
+				4 }, new int[] { 0,
+				1, 2, 3, 4 }));
+		for (int i = 0; i <= s.size(); i++) {
+			final IntIterator iterator = s.keySet().intIterator();
+			final int[] j = new int[1];
+			for (j[0] = 0; j[0] < i; j[0]++) iterator.nextInt();
+			iterator.forEachRemaining(x -> {
+				if (x != j[0]++) throw new AssertionError();
+			});
+		}
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testSkipZeroAtStart() {
+		final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
+				1 }));
+		final IntIterator i = s.keySet().intIterator();
+		i.skip(0);
+		i.remove();
+	}
+
+	@Test
+	public void testSkipOneAtStart() {
+		final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
+				1 }));
+		final IntIterator i = s.keySet().intIterator();
+		i.skip(1);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(1), s.keySet());
+	}
+
+	@Test
+	public void testSkipBeyondEnd() {
+		final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
+				1 }));
+		final IntIterator i = s.keySet().intIterator();
+		i.skip(4);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(0), s.keySet());
+	}
 }
diff --git a/test/it/unimi/dsi/fastutil/ints/IntArraySetTest.java b/test/it/unimi/dsi/fastutil/ints/IntArraySetTest.java
@@ -51,7 +51,7 @@ public void testNullInEquals() {
 
 	@Test
 	public void testSet() {
-		for(int i = 0; i <= 1; i++) {
+		for (int i = 0; i <= 1; i++) {
 			final IntArraySet s = i == 0 ? new IntArraySet() : new IntArraySet(new int[i]);
 			assertTrue(s.add(1));
 			assertEquals(1 + i, s.size());
@@ -140,7 +140,7 @@ public void testRemove() {
 		iterator.nextInt();
 		iterator.nextInt();
 		iterator.remove();
-		assertEquals(44, iterator.nextInt ());
+		assertEquals(44, iterator.nextInt());
 		assertFalse(iterator.hasNext());
 		assertEquals(new IntArraySet(new int[] { 42, 44 }), set);
 	}
@@ -190,11 +190,57 @@ public void testOfUnchekedSingleton() {
 	public void testOfUnchekedDuplicatesNotDetected() {
 		// A IntArraySet in an invalid state that by spec we aren't checking for in this method.
 		final IntArraySet s = IntArraySet.ofUnchecked(0, 0);
-		assertEquals(new IntArraySet(new int[] { 0 , 0 }), s);
+		assertEquals(new IntArraySet(new int[] { 0, 0 }), s);
 	}
 
 	@Test
 	public void testZeroLengthToArray() {
 		assertSame(IntArrays.EMPTY_ARRAY, new IntArraySet().toIntArray());
 	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testRemovalAtStart() {
+		final IntArraySet s = IntArraySet.ofUnchecked(0, 1);
+		final IntIterator i = s.intIterator();
+		i.remove();
+	}
+
+	@Test
+	public void testForEachRemaining() {
+		final IntArraySet s = IntArraySet.ofUnchecked(0, 1, 2, 3, 4);
+		for (int i = 0; i <= s.size(); i++) {
+			final IntIterator iterator = s.intIterator();
+			final int[] j = new int[1];
+			for(j[0] = 0; j[0] < i; j[0]++) iterator.nextInt();
+			iterator.forEachRemaining(x -> {
+				if (x != j[0]++) throw new AssertionError();
+			});
+		}
+	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testSkipZeroAtStart() {
+		final IntArraySet s = IntArraySet.ofUnchecked(0, 1);
+		final IntIterator i = s.intIterator();
+		i.skip(0);
+		i.remove();
+	}
+
+	@Test
+	public void testSkipOneAtStart() {
+		final IntArraySet s = IntArraySet.ofUnchecked(0, 1);
+		final IntIterator i = s.intIterator();
+		i.skip(1);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(1), s);
+	}
+
+	@Test
+	public void testSkipBeyondEnd() {
+		final IntArraySet s = IntArraySet.ofUnchecked(0, 1);
+		final IntIterator i = s.intIterator();
+		i.skip(4);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(0), s);
+	}
 }
diff --git a/test/it/unimi/dsi/fastutil/objects/Object2ObjectArrayMapTest.java b/test/it/unimi/dsi/fastutil/objects/Object2ObjectArrayMapTest.java
@@ -28,6 +28,7 @@
 
 import org.junit.Test;
 
+import it.unimi.dsi.fastutil.ints.IntArraySet;
 import it.unimi.dsi.fastutil.io.BinIO;
 
 public class Object2ObjectArrayMapTest  {
@@ -154,4 +155,51 @@ public void testIteratorRemove() {
 		assertEquals(Integer.valueOf(3), next.getKey());
 		assertEquals(Integer.valueOf(3), next.getValue());
 	}
+
+	@SuppressWarnings("boxing")
+	@Test
+	public void testForEachRemaining() {
+		final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
+				0, 1, 2, 3, 4 }, new Integer[] { 0, 1, 2, 3, 4 }));
+		for (int i = 0; i <= s.size(); i++) {
+			final ObjectIterator<Integer> iterator = s.keySet().iterator();
+			final int[] j = new int[1];
+			for (j[0] = 0; j[0] < i; j[0]++) iterator.next();
+			iterator.forEachRemaining(x -> {
+				if (x.intValue() != j[0]++) throw new AssertionError();
+			});
+		}
+	}
+
+	@SuppressWarnings("boxing")
+	@Test(expected = IllegalStateException.class)
+	public void testSkipZeroAtStart() {
+		final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
+				0, 1 }, new Integer[] { 0, 1 }));
+		final ObjectIterator<Integer> i = s.keySet().iterator();
+		i.skip(0);
+		i.remove();
+	}
+
+	@SuppressWarnings("boxing")
+	@Test
+	public void testSkipOneAtStart() {
+		final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
+				0, 1 }, new Integer[] { 0, 1 }));
+		final ObjectIterator<Integer> i = s.keySet().iterator();
+		i.skip(1);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(1), s.keySet());
+	}
+
+	@SuppressWarnings("boxing")
+	@Test
+	public void testSkipBeyondEnd() {
+		final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
+				0, 1 }, new Integer[] { 0, 1 }));
+		final ObjectIterator<Integer> i = s.keySet().iterator();
+		i.skip(4);
+		i.remove();
+		assertEquals(IntArraySet.ofUnchecked(0), s.keySet());
+	}
 }
diff --git a/test/it/unimi/dsi/fastutil/objects/ObjectArraySetTest.java b/test/it/unimi/dsi/fastutil/objects/ObjectArraySetTest.java
@@ -239,4 +239,43 @@ public void testOfUnchekedDuplicatesNotDetected() {
 	public void testZeroLengthToArray() {
 		assertSame(ObjectArrays.EMPTY_ARRAY, new ObjectArraySet<Integer>().toArray());
 	}
+
+	@Test(expected = IllegalStateException.class)
+	public void testRemovalAtStart() {
+		final ObjectArraySet<String> s = ObjectArraySet.ofUnchecked("0", "1");
+		final ObjectIterator<String> i = s.iterator();
+		i.remove();
+	}
+
+	@Test
+	public void testForEachRemaining() {
+		final ObjectArraySet<String> s = ObjectArraySet.ofUnchecked("0", "1", "2", "3", "4");
+		for (int i = 0; i <= s.size(); i++) {
+			final Iterator<String> iterator = s.iterator();
+			final int[] j = new int[1];
+			for (j[0] = 0; j[0] < i; j[0]++) iterator.next();
+			iterator.forEachRemaining(x -> {
+				if (!x.equals(String.valueOf(j[0]++))) throw new AssertionError();
+			});
+		}
+	}
+
+	@Test
+	public void testSkipOneAtStart() {
+		final ObjectArraySet<String> s = ObjectArraySet.ofUnchecked("0", "1");
+		final ObjectIterator<String> i = s.iterator();
+		i.skip(1);
+		i.remove();
+		assertEquals(ObjectArraySet.ofUnchecked("1"), s);
+	}
+
+	@Test
+	public void testSkipBeyondEnd() {
+		final ObjectArraySet<String> s = ObjectArraySet.ofUnchecked("0", "1");
+		final ObjectIterator<String> i = s.iterator();
+		i.skip(4);
+		i.remove();
+		assertEquals(ObjectArraySet.ofUnchecked("0"), s);
+	}
 }
+