Skip to content

Commit

Permalink
Fixed several problems with iterators of array-based maps
Browse files Browse the repository at this point in the history
  • Loading branch information
@vigna
vigna committed Jun 17, 2024
1 parent defd2d1 commit 3e59d8a
Show file tree
Hide file tree
Showing 6 changed files with 199 additions and 27 deletions.
4 changes: 4 additions & 0 deletions CHANGES
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
exception and leave the iterator in an inconsistent state when removing
before iterating. Thanks to Michal Frajt for reporting this bug.

- Entry.setValue() now works correctly in all iterators and iterator-like
methods of array-based maps. It was previously throwing an
UnsupportedOperationException.

8.5.13

- Thanks to Chanoch Goldfeder for fixing a number of bugs in ImmutableList.
Expand Down
136 changes: 114 additions & 22 deletions drv/ArrayMap.drv
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,113 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
if (size > key.length) throw new IllegalArgumentException("The provided size (" + size + ") is larger than or equal to the backing-arrays size (" + key.length + ")");
}

private final class EntrySet extends AbstractObjectSet<MAP.Entry KEY_VALUE_GENERIC> implements FastEntrySet KEY_VALUE_GENERIC {
/** The entry class for an array map does not record key and value, but
* rather the position in the array of the corresponding entry. This
* is necessary so that calls to {@link java.util.Map.Entry#setValue(Object)}
* are reflected in the map */

private final class MapEntry implements MAP.Entry KEY_VALUE_GENERIC, Map.Entry<KEY_GENERIC_CLASS, VALUE_GENERIC_CLASS>, PAIR KEY_VALUE_GENERIC {
// The array index this entry refers to, or -1 if this entry has been deleted.
int index;

MapEntry() {}

MapEntry(final int index) {
this.index = index;
}

@Override
SUPPRESS_WARNINGS_KEY_UNCHECKED
public KEY_GENERIC_TYPE ENTRY_GET_KEY() {
return KEY_GENERIC_CAST key[index];
}

@Override
SUPPRESS_WARNINGS_KEY_UNCHECKED
public KEY_GENERIC_TYPE PAIR_LEFT() {
return KEY_GENERIC_CAST key[index];
}

@Override
SUPPRESS_WARNINGS_VALUE_UNCHECKED
public VALUE_GENERIC_TYPE ENTRY_GET_VALUE() {
return VALUE_GENERIC_CAST value[index];
}

@Override
SUPPRESS_WARNINGS_VALUE_UNCHECKED
public VALUE_GENERIC_TYPE PAIR_RIGHT() {
return VALUE_GENERIC_CAST value[index];
}

@Override
SUPPRESS_WARNINGS_VALUE_UNCHECKED
public VALUE_GENERIC_TYPE setValue(final VALUE_GENERIC_TYPE v) {
final VALUE_GENERIC_TYPE oldValue = VALUE_GENERIC_CAST value[index];
value[index] = v;
return oldValue;
}

@Override
public PAIR KEY_VALUE_GENERIC right(final VALUE_GENERIC_TYPE v) {
value[index] = v;
return this;
}

#if KEYS_PRIMITIVE
/** {@inheritDoc}
* @deprecated Please use the corresponding type-specific method instead. */
@Deprecated
@Override
public KEY_GENERIC_CLASS getKey() {
return KEY2OBJ(key[index]);
}
#endif

#if VALUES_PRIMITIVE
/** {@inheritDoc}
* @deprecated Please use the corresponding type-specific method instead. */
@Deprecated
@Override
public VALUE_GENERIC_CLASS getValue() {
return VALUE2OBJ(value[index]);
}

/** {@inheritDoc}
* @deprecated Please use the corresponding type-specific method instead. */
@Deprecated
@Override
public VALUE_GENERIC_CLASS setValue(final VALUE_GENERIC_CLASS v) {
return VALUE2OBJ(setValue(VALUE_CLASS2TYPE(v)));
}
#endif

@SuppressWarnings("unchecked")
@Override
public boolean equals(final Object o) {
if (!(o instanceof Map.Entry)) return false;
Map.Entry<KEY_GENERIC_CLASS, VALUE_GENERIC_CLASS> e = (Map.Entry<KEY_GENERIC_CLASS, VALUE_GENERIC_CLASS>)o;

return KEY_EQUALS(key[index], KEY_CLASS2TYPE(e.getKey())) && VALUE_EQUALS(value[index], VALUE_CLASS2TYPE(e.getValue()));
}

@Override
public int hashCode() {
return KEY2JAVAHASH(key[index]) ^ VALUE2JAVAHASH(value[index]);
}

@Override
public String toString() {
return key[index] + "=>" + value[index];
}
}

// TODO Maybe make this return a list-iterator like the LinkedXHashMaps do? (same for other collection view types)

private final class EntrySet extends AbstractObjectSet<MAP.Entry KEY_VALUE_GENERIC> implements FastEntrySet KEY_VALUE_GENERIC {
@Override
public ObjectIterator<MAP.Entry KEY_VALUE_GENERIC> iterator() {
return new ObjectIterator<MAP.Entry KEY_VALUE_GENERIC>() {
private MapEntry entry;
int curr = -1, next = 0;

@Override
Expand All @@ -156,7 +256,7 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public Entry KEY_VALUE_GENERIC next() {
if (! hasNext()) throw new NoSuchElementException();
return new ABSTRACT_MAP.BasicEntry KEY_VALUE_GENERIC_DIAMOND(KEY_GENERIC_CAST key[curr = next], VALUE_GENERIC_CAST value[next++]);
return entry = new MapEntry(curr = next++);
}

@Override
Expand All @@ -166,6 +266,7 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
final int tail = size-- - next--;
System.arraycopy(key, next + 1, key, next, tail);
System.arraycopy(value, next + 1, value, next, tail);
entry.index = -1;
#if KEYS_REFERENCE
key[size] = null;
#endif
Expand All @@ -186,11 +287,10 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
@Override
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public void forEachRemaining(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {
final KEY_TYPE[] key = ARRAY_MAP.this.key;
final VALUE_TYPE[] value = ARRAY_MAP.this.value;
final int max = size;
while (next < max) {
action.accept(new ABSTRACT_MAP.BasicEntry KEY_VALUE_GENERIC_DIAMOND(KEY_GENERIC_CAST key[curr = next], VALUE_GENERIC_CAST value[next++]));
entry = new MapEntry(curr = next++);
action.accept(entry);
}
}
};
Expand All @@ -199,8 +299,8 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
@Override
public ObjectIterator<MAP.Entry KEY_VALUE_GENERIC> fastIterator() {
return new ObjectIterator<MAP.Entry KEY_VALUE_GENERIC>() {
private MapEntry entry = new MapEntry();
int next = 0, curr = -1;
final BasicEntry KEY_VALUE_GENERIC entry = new BasicEntry KEY_VALUE_GENERIC_DIAMOND ();

@Override
public boolean hasNext() { return next < size; }
Expand All @@ -209,8 +309,7 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public Entry KEY_VALUE_GENERIC next() {
if (! hasNext()) throw new NoSuchElementException();
entry.key = KEY_GENERIC_CAST key[curr = next];
entry.value = VALUE_GENERIC_CAST value[next++];
entry.index = curr = next++;
return entry;
}

Expand All @@ -221,6 +320,7 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
final int tail = size-- - next--;
System.arraycopy(key, next + 1, key, next, tail);
System.arraycopy(value, next + 1, value, next, tail);
entry.index = -1;
#if KEYS_REFERENCE
key[size] = null;
#endif
Expand All @@ -242,12 +342,9 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
@Override
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public void forEachRemaining(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {
final KEY_TYPE[] key = ARRAY_MAP.this.key;
final VALUE_TYPE[] value = ARRAY_MAP.this.value;
final int max = size;
while (next < max) {
entry.key = KEY_GENERIC_CAST key[curr = next];
entry.value = VALUE_GENERIC_CAST value[next++];
entry.index = curr = next++;
action.accept(entry);
}
}
Expand All @@ -271,7 +368,7 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
@Override
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
protected final MAP.Entry KEY_VALUE_GENERIC get(int location) {
return new ABSTRACT_MAP.BasicEntry KEY_VALUE_GENERIC_DIAMOND(KEY_GENERIC_CAST key[location], VALUE_GENERIC_CAST value[location]);
return new MapEntry(location);
}

@Override
Expand All @@ -289,23 +386,18 @@ public class ARRAY_MAP KEY_VALUE_GENERIC extends ABSTRACT_MAP KEY_VALUE_GENERIC
@Override
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public void forEach(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {
final KEY_TYPE[] key = ARRAY_MAP.this.key;
final VALUE_TYPE[] value = ARRAY_MAP.this.value;
for (int i = 0, max = size; i < max; ++i) {
action.accept(new ABSTRACT_MAP.BasicEntry KEY_VALUE_GENERIC_DIAMOND(KEY_GENERIC_CAST key[i], VALUE_GENERIC_CAST value[i]));
action.accept(new MapEntry(i));
}
}

/** {@inheritDoc} */
@Override
SUPPRESS_WARNINGS_KEY_VALUE_UNCHECKED
public void fastForEach(final Consumer<? super MAP.Entry KEY_VALUE_GENERIC> action) {
final KEY_TYPE[] key = ARRAY_MAP.this.key;
final VALUE_TYPE[] value = ARRAY_MAP.this.value;
final BasicEntry KEY_VALUE_GENERIC entry = new BasicEntry KEY_VALUE_GENERIC_DIAMOND ();
final MapEntry entry = new MapEntry();
for (int i = 0, max = size; i < max; ++i) {
entry.key = KEY_GENERIC_CAST key[i];
entry.value = VALUE_GENERIC_CAST value[i];
entry.index = i;
action.accept(entry);
}
}
Expand Down
2 changes: 1 addition & 1 deletion drv/ArraySet.drv
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public class ARRAY_SET KEY_GENERIC extends ABSTRACT_SET KEY_GENERIC implements j
@Override
public KEY_GENERIC_TYPE NEXT_KEY() {
if (! hasNext()) throw new NoSuchElementException();
return KEY_GENERIC_CAST a[next++];
return KEY_GENERIC_CAST a[curr = next++];
}

@Override
Expand Down
43 changes: 40 additions & 3 deletions test/it/unimi/dsi/fastutil/ints/Int2IntArrayMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@

import org.junit.Test;

import it.unimi.dsi.fastutil.objects.ObjectIterator;

public class Int2IntArrayMapTest {
@Test
public void testCopyConstructor() {
final Int2IntOpenHashMap m = new Int2IntOpenHashMap(new int [] {1, 2}, new int[] {3, 4});
final Int2IntOpenHashMap m = new Int2IntOpenHashMap(new int[] { 1, 2 }, new int[] { 3, 4 });
assertEquals(new Int2IntArrayMap(m), m);
assertEquals(new HashMap<>(m), m);
}
Expand Down Expand Up @@ -56,8 +58,7 @@ public void testValuesRemoveAll() {
@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 }));
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];
Expand Down Expand Up @@ -96,4 +97,40 @@ public void testSkipBeyondEnd() {
i.remove();
assertEquals(IntArraySet.ofUnchecked(0), s.keySet());
}

@Test
public void testSetValue() {
final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
1 }));
final ObjectIterator<Int2IntMap.Entry> i = s.int2IntEntrySet().iterator();
final Int2IntMap.Entry e = i.next();
assertEquals(e.setValue(1), 0);
assertEquals(e.setValue(0), 1);

final ObjectIterator<Int2IntMap.Entry> j = s.int2IntEntrySet().fastIterator();
final Int2IntMap.Entry f = j.next();
assertEquals(f.setValue(1), 0);
assertEquals(f.setValue(0), 1);
}

@Test(expected = ArrayIndexOutOfBoundsException.class)
public void testSetValueRemoved() {
final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
1 }));
final ObjectIterator<Int2IntMap.Entry> i = s.int2IntEntrySet().iterator();
final Int2IntMap.Entry e = i.next();
i.remove();
assertEquals(e.setValue(1), 0);
}

@Test(expected = ArrayIndexOutOfBoundsException.class)
public void testSetValueFastRemoved() {
final Int2IntArrayMap s = new Int2IntArrayMap(new Int2IntLinkedOpenHashMap(new int[] { 0, 1 }, new int[] { 0,
1 }));
final ObjectIterator<Int2IntMap.Entry> j = s.int2IntEntrySet().fastIterator();
final Int2IntMap.Entry f = j.next();
j.remove();
assertEquals(f.setValue(1), 0);
}

}
1 change: 0 additions & 1 deletion test/it/unimi/dsi/fastutil/ints/Int2IntOpenHashMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,5 +258,4 @@ public void testSetValue() {
s.int2IntEntrySet().fastForEach(e -> e.setValue(e.getIntValue() + 20));
for (int i = 0; i < 20; i++) assertEquals(20 + i, s.get(i));
}

}
40 changes: 40 additions & 0 deletions test/it/unimi/dsi/fastutil/objects/Object2ObjectArrayMapTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,4 +202,44 @@ public void testSkipBeyondEnd() {
i.remove();
assertEquals(IntArraySet.ofUnchecked(0), s.keySet());
}

@SuppressWarnings("boxing")
@Test
public void testSetValue() {
final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
0, 1 }, new Integer[] { 0, 1 }));
final ObjectIterator<Entry<Integer, Integer>> i = s.entrySet().iterator();
final Entry<Integer, Integer> e = i.next();
assertEquals(e.setValue(Integer.valueOf(1)), Integer.valueOf(0));
assertEquals(e.setValue(Integer.valueOf(0)), Integer.valueOf(1));

final ObjectIterator<Object2ObjectMap.Entry<Integer, Integer>> j = s.object2ObjectEntrySet().fastIterator();
final Entry<Integer, Integer> f = j.next();
assertEquals(f.setValue(Integer.valueOf(1)), Integer.valueOf(0));
assertEquals(f.setValue(Integer.valueOf(0)), Integer.valueOf(1));
}

@SuppressWarnings("boxing")
@Test(expected = ArrayIndexOutOfBoundsException.class)
public void testSetValueRemoved() {
final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
0, 1 }, new Integer[] { 0, 1 }));
final ObjectIterator<Entry<Integer, Integer>> i = s.entrySet().iterator();
final Entry<Integer, Integer> e = i.next();
i.remove();
assertEquals(e.setValue(Integer.valueOf(1)), Integer.valueOf(0));

}

@SuppressWarnings("boxing")
@Test(expected = ArrayIndexOutOfBoundsException.class)
public void testSetValueFastRemoved() {
final Object2ObjectArrayMap<Integer, Integer> s = new Object2ObjectArrayMap<>(new Object2ObjectLinkedOpenHashMap<>(new Integer[] {
0, 1 }, new Integer[] { 0, 1 }));

final ObjectIterator<Object2ObjectMap.Entry<Integer, Integer>> j = s.object2ObjectEntrySet().fastIterator();
final Entry<Integer, Integer> f = j.next();
j.remove();
assertEquals(f.setValue(Integer.valueOf(1)), Integer.valueOf(0));
}
}

0 comments on commit 3e59d8a

Please sign in to comment.