Add documentation, adjust tests, and cleanup/fix behavior

src/main/java/net/neoforged/neoforge/common/util/InsertableLinkedOpenCustomHashSet.java

@@ -16,24 +16,43 @@
  * Requires a strategy for the hashing behavior. Use {@link BasicStrategy#BASIC} or {@link IdentityStrategy#IDENTITY} if no special hashing needed.
  */
 public class InsertableLinkedOpenCustomHashSet<T> extends ObjectLinkedOpenCustomHashSet<T> {
-    private static final long LINK_BITS = 0xFFFFFFFFL;
+    /** The number of bits which take up the space for the previous or next position. */
     private static final long LINK_BIT_SPACE = 32L;
-    private static final long NEXT_LINK = LINK_BITS;
-    private static final long PREV_LINK = LINK_BITS << LINK_BIT_SPACE;
+    /** The bitmask for the next element's position. */
+    private static final long NEXT_LINK = 0x00000000FFFFFFFFL;
+    /** The bitmask for the previous element's position. */
+    private static final long PREV_LINK = 0xFFFFFFFF00000000L;
 
+    /**
+     * Constructs a new {@link InsertableLinkedOpenCustomHashSet} with a {@link BasicStrategy}.
+     */
     public InsertableLinkedOpenCustomHashSet() {
         super(BasicStrategy.BASIC);
     }
 
+    /**
+     * Constructs a new {@link InsertableLinkedOpenCustomHashSet} with the given {@link Hash.Strategy}.
+     * @param strategy The strategy to use for adding and getting elements from the set.
+     */
     public InsertableLinkedOpenCustomHashSet(Hash.Strategy<? super T> strategy) {
         super(strategy);
     }
 
+    /**
+     * This method will attempt to add {@code element} after the given element {@code insertAfter} in the set. If an
+     * element matching {@code insertAfter} cannot be found with this set's {@link Hash.Strategy}, then {@code element}
+     * will be added in insertion order. If {#code element} already exists in the set, then the set is not modified.
+     * @param insertAfter The element to insert {@code element} after.
+     * @param element The element to add into this set.
+     * @return {@code true} if the element was added to the set.
+     */
     public boolean addAfter(T insertAfter, T element) {
         int afterPos;
+        // Use the default return if afterPos == last. Otherwise, special checks would have to be done.
         if (contains(insertAfter) && (last != (afterPos = getPos(insertAfter)))) {
             int pos = HashCommon.mix(strategy.hashCode(element)) & mask;
             T curr = key[pos];
+            // If an element exists in this pos, shift index until an empty space is found or return false if it matches.
             if (curr != null) {
                 do {
                     if (strategy.equals(curr, element)) {
@@ -43,12 +62,15 @@ public boolean addAfter(T insertAfter, T element) {
             }
             key[pos] = element;
 
+            // This chunk inserts the new pos in-between insertAfter and it's next link.
             long nextPos = link[afterPos] & NEXT_LINK;
-            if (nextPos != LINK_BITS) {
-                link[(int) nextPos] ^= (link[(int) nextPos] ^ ((pos & NEXT_LINK) << LINK_BIT_SPACE)) & PREV_LINK;
-            }
+            // Fix the previous link for the next element to point back to this pos.
+            link[(int) nextPos] ^= (link[(int) nextPos] ^ ((pos & NEXT_LINK) << LINK_BIT_SPACE)) & PREV_LINK;
+            // Fix the next link for insertAfter to point forward to this pos.
             link[afterPos] ^= (link[afterPos] ^ (pos & NEXT_LINK)) & NEXT_LINK;
+            // Set this pos to point back to insertAfter and forward to the next element.
             link[pos] = ((afterPos & NEXT_LINK) << LINK_BIT_SPACE) | nextPos;
+
             if (size++ >= maxFill) {
                 rehash(HashCommon.arraySize(size + 1, f));
             }
@@ -57,14 +79,27 @@ public boolean addAfter(T insertAfter, T element) {
         return add(element);
     }
 
+    /**
+     * This method will attempt to add {@code element} before the given element {@code insertBefore} in the set. If an
+     * element matching {@code insertBefore} cannot be found with this set's {@link Hash.Strategy}, then {@code element}
+     * will be added in insertion order. If {#code element} already exists in the set, then the set is not modified.
+     * @param insertBefore The element to insert {@code element} before.
+     * @param element The element to add into this set.
+     * @return {@code true} if the element was added to the set.
+     */
     public boolean addBefore(T insertBefore, T element) {
         if (contains(insertBefore)) {
             int beforePos = getPos(insertBefore);
+            // Use this method instead so special logic isn't needed for handling invalid "previous" links.
             if (beforePos == first) {
+                if (contains(element)) {
+                    return false;
+                }
                 return addAndMoveToFirst(element);
             }
             int pos = HashCommon.mix(strategy.hashCode(element)) & mask;
             T curr = key[pos];
+            // If an element exists in this pos, shift index until an empty space is found or return false if it matches.
             if (curr != null) {
                 do {
                     if (strategy.equals(curr, element)) {
@@ -74,12 +109,15 @@ public boolean addBefore(T insertBefore, T element) {
             }
             key[pos] = element;
 
-            long prevPos = (link[beforePos] & PREV_LINK) >> LINK_BIT_SPACE;
-            if (prevPos != LINK_BITS && prevPos != -1) {
-                link[(int) prevPos] ^= (link[(int) prevPos] ^ (pos & NEXT_LINK)) & NEXT_LINK;
-            }
+            // This chunk inserts the new pos in-between insertBefore and it's previous link.
+            long prevPos = ((link[beforePos] & PREV_LINK) >> LINK_BIT_SPACE);
+            // Fix the next link for the previous element to point forward to this pos.
+            link[(int) prevPos] ^= (link[(int) prevPos] ^ (pos & NEXT_LINK)) & NEXT_LINK;
+            // Fix the previous link for insertBefore to point back to this pos.
             link[beforePos] ^= (link[beforePos] ^ ((pos & NEXT_LINK) << LINK_BIT_SPACE)) & PREV_LINK;
+            // Set this pos to point back to the previous element and forward to insertBefore.
             link[pos] = (prevPos << LINK_BIT_SPACE) | (beforePos & NEXT_LINK);
+
             if (size++ >= maxFill) {
                 rehash(HashCommon.arraySize(size + 1, f));
             }
@@ -99,7 +137,7 @@ public void addLast(T element) {
     }
 
     /**
-     * Requires that insertAfter exists in the set already.
+     * Requires that {@code existingElement} exists in the set already.
      */
     private int getPos(T existingElement) {
         int pos = HashCommon.mix(strategy.hashCode(existingElement)) & mask;

src/main/java/net/neoforged/neoforge/common/util/strategy/BasicStrategy.java

@@ -8,10 +8,10 @@
 import it.unimi.dsi.fastutil.Hash;
 import java.util.Objects;
 
+/**
+ * A strategy that uses {@link Objects#hashCode(Object)} and {@link Object#equals(Object)}.
+ */
 public class BasicStrategy implements Hash.Strategy<Object> {
-    /**
-     * A strategy that uses {@link Objects#hashCode(Object)} and {@link Object#equals(Object)}.
-     */
     public static final Hash.Strategy<? super Object> BASIC = new BasicStrategy();
 
     @Override

src/main/java/net/neoforged/neoforge/common/util/strategy/IdentityStrategy.java

@@ -7,10 +7,10 @@
 
 import it.unimi.dsi.fastutil.Hash;
 
+/**
+ * A strategy that uses {@link System#identityHashCode(Object)} and {@code a == b} comparisons.
+ */
 public class IdentityStrategy implements Hash.Strategy<Object> {
-    /**
-     * A strategy that uses {@link System#identityHashCode(Object)} and {@code a == b} comparisons.
-     */
     public static final Hash.Strategy<? super Object> IDENTITY = new IdentityStrategy();
 
     @Override

tests/src/junit/java/net/neoforged/neoforge/unittest/CreativeTabOrderTest.java

@@ -91,7 +91,7 @@ static void testSetupTabs(MinecraftServer server) {
     }
 
     /**
-     * The local tabEnchantments variable comes from {@link CreativeModeTabs#generateEnchantmentBookTypesOnlyMaxLevel(CreativeModeTab.Output, HolderLookup, Set, CreativeModeTab.TabVisibility, FeatureFlagSet)}
+     * The local tabEnchantments variable comes from {@link CreativeModeTabs#generateEnchantmentBookTypesOnlyMaxLevel}
      * 
      * @param server Ephemeral server from extension
      */
@@ -110,7 +110,7 @@ void testIngredientsEnchantmentExistence(MinecraftServer server) {
     }
 
     /**
-     * The local tabEnchantments variable comes from {@link CreativeModeTabs#generateEnchantmentBookTypesAllLevels(CreativeModeTab.Output, HolderLookup, Set, CreativeModeTab.TabVisibility, FeatureFlagSet)}
+     * The local tabEnchantments variable comes from {@link CreativeModeTabs#generateEnchantmentBookTypesAllLevels}
      * 
      * @param server Ephemeral server from extension
      */

tests/src/junit/java/net/neoforged/neoforge/unittest/InsertableLinkedOpenCustomHashSetTest.java

@@ -6,38 +6,76 @@
 package net.neoforged.neoforge.unittest;
 
 import java.util.stream.IntStream;
+
+import it.unimi.dsi.fastutil.Hash;
+import it.unimi.dsi.fastutil.objects.ObjectLinkedOpenHashSet;
 import net.neoforged.neoforge.common.util.InsertableLinkedOpenCustomHashSet;
+import org.jetbrains.annotations.Nullable;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 public class InsertableLinkedOpenCustomHashSetTest {
+    private static final Hash.Strategy<? super String> CUSTOM_STRATEGY = new Hash.Strategy<>() {
+        @Override
+        public int hashCode(String o) {
+            return Character.hashCode(o.charAt(0));
+        }
+
+        @Override
+        public boolean equals(@Nullable String a, @Nullable String b) {
+            return a == null ? b == null : b != null && a.charAt(0) == b.charAt(0);
+        }
+    };
+
     @Test
     public void testAddAfter() {
-        InsertableLinkedOpenCustomHashSet<Integer> test = new InsertableLinkedOpenCustomHashSet<>();
+        var test = new InsertableLinkedOpenCustomHashSet<Integer>();
         IntStream.rangeClosed(0, 10).forEach(test::add);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10), test);
         test.addAfter(4, 100);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 100, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 100, 5, 6, 7, 8, 9, 10), test);
         test.addAfter(100, 101);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10), test);
         test.addAfter(10, 102);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10, 102 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10, 102), test);
         test.addAfter(-1, 103);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10, 102, 103 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 100, 101, 5, 6, 7, 8, 9, 10, 102, 103), test);
+        Assertions.assertFalse(test.addAfter(3, 103), "Set was mutated, despite the element being present");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10, 103), test);
     }
 
     @Test
     public void testAddBefore() {
-        InsertableLinkedOpenCustomHashSet<Integer> test = new InsertableLinkedOpenCustomHashSet<>();
+        var test = new InsertableLinkedOpenCustomHashSet<Integer>();
         IntStream.rangeClosed(0, 10).forEach(test::add);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10), test);
         test.addBefore(4, 100);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 100, 4, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 100, 4, 5, 6, 7, 8, 9, 10), test);
         test.addBefore(100, 101);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10), test);
         test.addBefore(0, 102);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10), test);
         test.addBefore(-1, 103);
-        Assertions.assertArrayEquals(test.toArray(), new Integer[] { 102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10, 103 });
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10, 103), test);
+        Assertions.assertFalse(test.addBefore(3, 103), "Set was mutated, despite the element being present");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of(102, 0, 1, 2, 3, 101, 100, 4, 5, 6, 7, 8, 9, 10, 103), test);
+    }
+
+    @Test
+    public void testAddAfterCustomStrategy() {
+        var test = new InsertableLinkedOpenCustomHashSet<String>(CUSTOM_STRATEGY);
+        test.add("here");
+        test.add("is");
+        test.add("a");
+        test.add("test");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of("here", "is", "a", "test"), test);
+        test.addAfter("a", "b");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of("here", "is", "a", "b", "test"), test);
+        test.addAfter("b", "c");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of("here", "is", "a", "b", "c", "test"), test);
+        test.addAfter("test", "102");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of("here", "is", "a", "b", "c", "test", "102"), test);
+        test.addAfter("doesn't exist", "203");
+        Assertions.assertEquals(ObjectLinkedOpenHashSet.of("here", "is", "a", "b", "c", "test", "102", "203"), test);
     }
 }