[jira] [Created] (COLLECTIONS-826) BloomFilter: move CountingLongPredicate

["Claude Warren (Jira)" <ji...@apache.org>]

Claude Warren created COLLECTIONS-826:
-----------------------------------------

             Summary: BloomFilter: move CountingLongPredicate
                 Key: COLLECTIONS-826
                 URL: https://issues.apache.org/jira/browse/COLLECTIONS-826
             Project: Commons Collections
          Issue Type: Improvement
          Components: Collection
    Affects Versions: 4.5
            Reporter: Claude Warren


 
{noformat}
    class CountingLongPredicate implements LongPredicate {
        int idx = 0;
        final long[] ary;
        final LongBiPredicate func;

        CountingLongPredicate(long[] ary, LongBiPredicate func) {
            this.ary = ary;
            this.func = func;
        }

        @Override
        public boolean test(long other) {
            return func.test(idx == ary.length ? 0 : ary[idx++], other);
        }

        boolean forEachRemaining() {
            while (idx != ary.length && func.test(ary[idx], 0)) {
                idx++;
            }
            return idx == ary.length;
        }
    }
{noformat}
 


Since this is in the interface it is public. It should be an implementation detail. Hiding it ensures it is a one time use only and prevents strangeness when calling forEachRemaining (see below).

idx will be initialsed as zero anyway so this can be removed: int idx = 0;


 @aherbert aherbert on 27 Feb

If func.test returns false the loop will terminate before idx == ary.length. So we return false. However this allows forEachRemaining to be called again. This will use the same position in the array again. If public (currently it is not) then the method could be invoked multiple times and the behaviour is not defined (especially since you have added no comment on this).

However the class is public but the forEachRemaining method is package private. I suggest moving this out of the interface so the class is package private.

Note: I tested a spliterator from the List returned from Arrays.asList. The forEachRemaining can be invoked one time only. This behaviour can be obtained by caching idx and setting it to the array length:


{noformat}
boolean forEachRemaining() {
    int i = idx;
    final long[] a = ary;
    final int limit = a.length;
    if (i != limit) {
        // Prevent subsequent calls
        idx = limit;
        while (i != limit && func.test(a[i], 0)) {
            i++;
        }
    }
    return i == limit;
}

{noformat}

However the code above as it stands could fail-fast and return false. Then be invoked again, do nothing with the func but will return true. The idx could be set above limit but this will only work if limit is not Integer.MAX_VALUE (which is unlikely). This can be handled by using compared unsigned:

boolean forEachRemaining() {
    int i = idx;
    final long[] a = ary;
    final int limit = a.length;
    if (Integer.compareUnsigned(i, limit) < 0) {
        while (i != limit && func.test(a[i], 0)) {
            i++;
        }
        // Set the result for repeat calls
        idx = i == limit ? i : -1;
    }
    return i == limit;
}

This is over engineering a simple helper class. However a tiny optimisation to use local references would be of benefit. So make the class package private, add some javadoc and do:

{noformat}

/**
 * Call the long-long consuming bi-predicate for each remaining unpaired long in
 * the input array. This method should be invoked after the predicate has been
 * passed to {@link BitMapProducer#forEachBitMap(LongPredicate)} to consume any
 * unpaired bitmaps. The second argument to the bi-predicate will be zero.
 *
 * @return true if all calls the predicate were successful
 */
boolean forEachRemaining() {
    int i = idx;
    final long[] a = ary;
    final int limit = a.length;
    while (i != limit && func.test(a[i], 0)) {
        i++;
    }
    return i == limit;
}

{noformat}







--
This message was sent by Atlassian Jira
(v8.20.7#820007)