From 3a981a01b7637635025d2dde7d9f1142e25da07b Mon Sep 17 00:00:00 2001
From: Alex Herbert <aherbert@apache.org>
Date: Thu, 12 Mar 2020 22:44:23 +0000
Subject: [PATCH] Update BloomFilterIndex comments and added tests for negative
 index.

---
 .../bloomfilter/BloomFilterIndexer.java       |  3 +-
 .../bloomfilter/BloomFilterIndexerTest.java   | 54 ++++++++++++++++---
 2 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexer.java b/src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexer.java
index 22044fbd3..7066a6588 100644
--- a/src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexer.java
+++ b/src/main/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexer.java
@@ -77,7 +77,8 @@ final class BloomFilterIndexer {
     static long getLongBit(int bitIndex) {
         // Bit shifts only use the first 6 bits. Thus it is not necessary to mask this
         // using 0x3f (63) or compute bitIndex % 64.
-        // If the index is negative the shift will be in the opposite direction.
+        // Note: If the index is negative the shift will be (64 - (bitIndex & 0x3f)) and
+        // this will identify an incorrect bit.
         return 1L << bitIndex;
     }
 }
diff --git a/src/test/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexerTest.java b/src/test/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexerTest.java
index a55270624..cb9b08f84 100644
--- a/src/test/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexerTest.java
+++ b/src/test/java/org/apache/commons/collections4/bloomfilter/BloomFilterIndexerTest.java
@@ -19,6 +19,7 @@ package org.apache.commons.collections4.bloomfilter;
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.ArrayList;
 import java.util.Random;
 import java.util.concurrent.ThreadLocalRandom;
 
@@ -34,21 +35,58 @@ public class BloomFilterIndexerTest {
 
     @Test
     public void testGetLongIndex() {
-        final Random rng = ThreadLocalRandom.current();
-        for (int i = 0; i < 10; i++) {
-            // Random positive number
-            final int index = rng.nextInt() >>> 1;
+        Assert.assertEquals(0, BloomFilterIndexer.getLongIndex(0));
+
+        for (final int index : getIndexes()) {
+            // getLongIndex is expected to identify a block of 64-bits (starting from zero)
             Assert.assertEquals(index / Long.SIZE, BloomFilterIndexer.getLongIndex(index));
+
+            // Verify the behaviour for negatives. It should produce a negative (invalid)
+            // as a simple trip for incorrect usage.
+            Assert.assertTrue(BloomFilterIndexer.getLongIndex(-index) < 0);
+
+            // If index is not zero then when negated this is what a signed shift
+            // of 6-bits actually does
+            Assert.assertEquals(((1 - index) / Long.SIZE) - 1,
+                    BloomFilterIndexer.getLongIndex(-index));
         }
     }
 
     @Test
     public void testGetLongBit() {
-        final Random rng = ThreadLocalRandom.current();
-        for (int i = 0; i < 10; i++) {
-            // Random positive number
-            final int index = rng.nextInt() >>> 1;
+        Assert.assertEquals(1L, BloomFilterIndexer.getLongBit(0));
+
+        for (final int index : getIndexes()) {
+            // getLongBit is expected to identify a single bit in a 64-bit block
             Assert.assertEquals(1L << (index % Long.SIZE), BloomFilterIndexer.getLongBit(index));
+
+            // Verify the behaviour for negatives
+            Assert.assertEquals(1L << (64 - (index & 0x3f)), BloomFilterIndexer.getLongBit(-index));
         }
     }
+
+    /**
+     * Gets non-zero positive indexes for testing.
+     *
+     * @return the indices
+     */
+    private static int[] getIndexes() {
+        final Random rng = ThreadLocalRandom.current();
+        ArrayList<Integer> indexes = new ArrayList<>(40);
+        for (int i = 0; i < 10; i++) {
+            // random positive numbers
+            indexes.add(rng.nextInt() >>> 1);
+            indexes.add(rng.nextInt(23647826));
+            indexes.add(rng.nextInt(245));
+        }
+        // Quickly remove zeros (as these cannot be negated)
+        indexes.removeIf(i -> i == 0);
+        // Add edge cases here
+        indexes.add(1);
+        indexes.add(2);
+        indexes.add(63);
+        indexes.add(64);
+        indexes.add(Integer.MAX_VALUE);
+        return indexes.stream().mapToInt(Integer::intValue).toArray();
+    }
 }