From defd2d1fe36d328824478ef0914184b544bd21d3 Mon Sep 17 00:00:00 2001 From: Sebastiano Vigna Date: Fri, 14 Jun 2024 13:27:48 +0200 Subject: [PATCH] Fixed inconsistent behavior and added missing impls in iterator of array-based containers --- CHANGES | 7 +++ drv/ArrayMap.drv | 19 +++++++ drv/ArraySet.drv | 20 ++++--- .../dsi/fastutil/ints/IntSetGuavaTest.java | 1 + .../fastutil/ints/Int2IntArrayMapTest.java | 44 ++++++++++++++++ .../dsi/fastutil/ints/IntArraySetTest.java | 52 +++++++++++++++++-- .../objects/Object2ObjectArrayMapTest.java | 48 +++++++++++++++++ .../fastutil/objects/ObjectArraySetTest.java | 39 ++++++++++++++ 8 files changed, 219 insertions(+), 11 deletions(-) diff --git a/CHANGES b/CHANGES index deff05387..76fd6eff1 100644 --- 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 index c261a1c87..15bb2cbdc 100644 --- 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 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 action) { diff --git a/drv/ArraySet.drv b/drv/ArraySet.drv index b241866d0..8d2471e49 100644 --- 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 index 737576fba..64f0a3c53 100644 --- a/guava/it/unimi/dsi/fastutil/ints/IntSetGuavaTest.java +++ b/guava/it/unimi/dsi/fastutil/ints/IntSetGuavaTest.java @@ -72,4 +72,5 @@ public SampleElements 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 index c78d670bc..ac647fa47 100644 --- 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 index be0eeafd4..302c27f63 100644 --- 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 index bc83549fd..271eba344 100644 --- 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 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 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 s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] { + 0, 1 }, new Integer[] { 0, 1 })); + final ObjectIterator i = s.keySet().iterator(); + i.skip(0); + i.remove(); + } + + @SuppressWarnings("boxing") + @Test + public void testSkipOneAtStart() { + final Object2ObjectArrayMap s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] { + 0, 1 }, new Integer[] { 0, 1 })); + final ObjectIterator i = s.keySet().iterator(); + i.skip(1); + i.remove(); + assertEquals(IntArraySet.ofUnchecked(1), s.keySet()); + } + + @SuppressWarnings("boxing") + @Test + public void testSkipBeyondEnd() { + final Object2ObjectArrayMap s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] { + 0, 1 }, new Integer[] { 0, 1 })); + final ObjectIterator 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 index cd109a269..747a37b68 100644 --- 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().toArray()); } + + @Test(expected = IllegalStateException.class) + public void testRemovalAtStart() { + final ObjectArraySet s = ObjectArraySet.ofUnchecked("0", "1"); + final ObjectIterator i = s.iterator(); + i.remove(); + } + + @Test + public void testForEachRemaining() { + final ObjectArraySet s = ObjectArraySet.ofUnchecked("0", "1", "2", "3", "4"); + for (int i = 0; i <= s.size(); i++) { + final Iterator 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 s = ObjectArraySet.ofUnchecked("0", "1"); + final ObjectIterator i = s.iterator(); + i.skip(1); + i.remove(); + assertEquals(ObjectArraySet.ofUnchecked("1"), s); + } + + @Test + public void testSkipBeyondEnd() { + final ObjectArraySet s = ObjectArraySet.ofUnchecked("0", "1"); + final ObjectIterator i = s.iterator(); + i.skip(4); + i.remove(); + assertEquals(ObjectArraySet.ofUnchecked("0"), s); + } } +