Backport PrimitiveHashSet#iterator()#remove() to not rehash. Fixes go…

…ldmansachs#35. Signed-off-by: Nikhil Nanivadekar <nikhil.nanivadekar@gs.com>
nikhilnanivadekar · Mar 25, 2018 · 8aed5e6 · 8aed5e6
1 parent 9d894f5
commit 8aed5e6
Show file tree

Hide file tree

Showing 2 changed files with 91 additions and 37 deletions.
diff --git a/gs-collections-code-generator/src/main/resources/impl/set/mutable/primitiveHashSet.stg b/gs-collections-code-generator/src/main/resources/impl/set/mutable/primitiveHashSet.stg
@@ -289,14 +289,7 @@ public final class <name>HashSet extends Abstract<name>Set implements Mutable<na
     {
         if (isBetweenZeroAndThirtyOne(value))
         {
-            int initial = this.zeroToThirtyOne;
-            this.zeroToThirtyOne &= ~(1 \<\< <(castRealTypeToInt.(type))("value")>);
-            if (this.zeroToThirtyOne == initial)
-            {
-                return false;
-            }
-            this.zeroToThirtyOneOccupied--;
-            return true;
+            return this.removeZeroToThirtyOne(value);
         }
         int index = this.probe(value);
         if (<(equals.(type))("this.table[index]", "value")>)
@@ -318,6 +311,18 @@ public final class <name>HashSet extends Abstract<name>Set implements Mutable<na
         return false;
     }
 
+    private boolean removeZeroToThirtyOne(<type> value)
+    {
+        int initial = this.zeroToThirtyOne;
+        this.zeroToThirtyOne &= ~(1 \<\< <(castRealTypeToInt.(type))("value")>);
+        if (this.zeroToThirtyOne == initial)
+        {
+            return false;
+        }
+        this.zeroToThirtyOneOccupied--;
+        return true;
+    }
+
     public boolean removeAll(<name>Iterable source)
     {
         if (source.isEmpty())
@@ -1606,32 +1611,46 @@ public final class <name>HashSet extends Abstract<name>Set implements Mutable<na
             return result;
         }
 
-        public void remove()
-        {
-            if (this.count == 0)
-            {
-                throw new IllegalStateException();
-            }
-            <type> removeValue;
-            if (this.zeroToThirtyOne \<= <(literal.(type))("32")> && this.position == 0)
-            {
-                if (<name>HashSet.this.zeroToThirtyOne != (<name>HashSet.this.zeroToThirtyOne | 1 \<\< (<(castRealTypeToInt.(type))("this.zeroToThirtyOne")> - 1)))
-                {
-                    throw new IllegalStateException();
-                }
-                removeValue = <(castIntToNarrowTypeWithParens.(type))("this.zeroToThirtyOne - 1")>;
-            }
-            else if (<(equals.(type))({<name>HashSet.this.table[this.position - 1]}, "REMOVED")>)
-            {
-                throw new IllegalStateException();
-            }
-            else
-            {
-                removeValue = <name>HashSet.this.table[this.position - 1];
-            }
-            <name>HashSet.this.remove(removeValue);
-            this.count--;
-        }
+         public void remove()
+         {
+             if (this.count == 0)
+             {
+                 throw new IllegalStateException();
+             }
+             <type> removeValue;
+             if (this.zeroToThirtyOne \<= <(literal.(type))("32")> && this.position == 0)
+             {
+                 if (<name>HashSet.this.zeroToThirtyOne != (<name>HashSet.this.zeroToThirtyOne | 1 \<\< (<(castRealTypeToInt.(type))("this.zeroToThirtyOne")> - 1)))
+                 {
+                     throw new IllegalStateException();
+                 }
+                 removeValue = <(castIntToNarrowTypeWithParens.(type))("this.zeroToThirtyOne - 1")>;
+             }
+             else if (<(equals.(type))({<name>HashSet.this.table[this.position - 1]}, "REMOVED")>)
+             {
+                 throw new IllegalStateException();
+             }
+             else
+             {
+                 removeValue = <name>HashSet.this.table[this.position - 1];
+             }
+             if (<name>HashSet.isBetweenZeroAndThirtyOne(removeValue))
+             {
+                 <name>HashSet.this.removeZeroToThirtyOne(removeValue);
+             }
+             else if (<(equals.(type))({<name>HashSet.this.table[this.position - 1]}, "removeValue")>)
+             {
+                 if (<name>HashSet.this.copyOnWrite)
+                 {
+                     <name>HashSet.this.copyTable();
+                 }
+                 <name>HashSet.this.table[position -1] = REMOVED;
+                 <name>HashSet.this.occupiedWithData--;
+                 <name>HashSet.this.occupiedWithSentinels++;
+             }
+
+             this.count--;
+         }
     }
 }
 
@@ -1716,4 +1735,4 @@ public <wideType.(type)> sum()
     return result;
 }
 
->>
+>>
diff --git a/gs-collections-code-generator/src/main/resources/test/set/mutable/primitiveHashSetTest.stg b/gs-collections-code-generator/src/main/resources/test/set/mutable/primitiveHashSetTest.stg
@@ -8,16 +8,18 @@ targetPath() ::= "com/gs/collections/impl/set/mutable/primitive"
 fileName(primitive) ::= "<primitive.name>HashSetTest"
 
 class(primitive) ::= <<
-<body(primitive.type, primitive.wrapperName, primitive.name)>
+<body(primitive.type, primitive.wrapperName, primitive.name, primitive.charPrimitive, primitive.shortPrimitive)>
 >>
 
-body(type, wrapperName, name) ::= <<
+body(type, wrapperName, name, charPrimitive, shortPrimitive) ::= <<
 <copyright()>
 
 package com.gs.collections.impl.set.mutable.primitive;
 
 import java.lang.reflect.Field;
 
+import com.gs.collections.api.iterator.Mutable<name>Iterator;
+import com.gs.collections.api.set.primitive.Mutable<name>Set;
 import com.gs.collections.impl.factory.primitive.<name>Sets;
 import com.gs.collections.impl.list.mutable.primitive.<name>ArrayList;
 import com.gs.collections.impl.test.Verify;
@@ -177,6 +179,39 @@ public class <name>HashSetTest extends Abstract<name>SetTestCase
         Assert.assertEquals(0, occupiedWithSentinels.get(hashSet));
     }
 
+    @Test
+    public void iterator_remove()
+    {
+        Mutable<name>Set set1 = <name>Sets.mutable.empty();
+
+        int <if(shortPrimitive)>max = 65_536<elseif(charPrimitive)>max = 10<else>max = 100_000<endif>;
+        for (Integer i = 0; i \< max; i++)
+        {
+            <if(charPrimitive)>set1.add(i.toString().charAt(0));<else>set1.add(i.<type>Value());<endif>
+        }
+
+        // set2 to verify copyTable()
+        Mutable<name>Set set2 = <name>Sets.mutable.withAll(set1);
+        set2.freeze();
+
+        this.assertIteratorRemove(set1, max);
+        this.assertIteratorRemove(set2, max);
+    }
+
+    private void assertIteratorRemove(Mutable<name>Set set, int max)
+    {
+        Verify.assertSize(max, set);
+        Mutable<name>Iterator iterator = set.<type>Iterator();
+        Verify.assertThrows(IllegalStateException.class, () -> iterator.remove());
+
+        while (iterator.hasNext())
+        {
+            iterator.next();
+            iterator.remove();
+            Verify.assertSize(--max, set);
+        }
+    }
+
     @Test
     public void addEverySlot()
     {