[SC-10749]

Summary

• Keep Range.iterator() backed by IntegerSequence.Incrementor.
• Fix counter overflow in Incrementor so range iteration and downstream callers using the incrementor get correct exhaustion behavior.
• Handle inclusive range endpoints within Incrementor, while preserving the public factory's existing exclusive-limit behavior.
• Add regression coverage for integer boundaries, bulk increments, iterator exhaustion, and callbacks.

Validation

• IntegerSequenceTest: 25 tests passed.
• commons-math-legacy-core: all 299 tests and Checkstyle, PMD, and SpotBugs checks passed.
• pnpm hotline validate: passed.
• Full mvn -B -ntp clean verify was attempted; UnivariatePeriodicInterpolatorTest.testSine failed in commons-math-legacy. The same failure was reproduced with the pre-PR IntegerSequence implementation.

[aherbert]

Thanks for the PR. The tests are valid. The code change should apply the fix to the Incrementor class. This change drops use of the Incrementor and creates a new Iterator implementation. Downstream classes that use the Incrementor will not benefit from the change.

[SC-10749]

Thanks for the PR. The tests are valid. The code change should apply the fix to the Incrementor class. This change drops use of the Incrementor and creates a new Iterator implementation. Downstream classes that use the Incrementor will not benefit from the change.

Thanks for your review and feedback! I understand your point.
I will revise the fix directly inside the Incrementor class instead of introducing a new Iterator implementation, so that all downstream classes relying on Incrementor can get this fix.
I'll push the updated changes soon.

[aherbert]

Thanks for the update. Using a long count is fine but we must be sure not to increment it beyond the maximal count. If we do then the getCount method should return the maximal count, not a 32-bit truncation of the long.

I've added some notes on possible changes to improve the class.

[aherbert]

This is existing strange behaviour in the IntegerSequence. It updates the member count then checks if any more are allowed. The purpose is to allow multiple increments when at least 1 increment is possible, and then check if multiple have exhausted the sequence to trigger the callback. But the sequence does not actually check if even 1 increment is possible. I would be inclined to refactor canIncrement to not call canincrement(1) but to directly check the current count:

public boolean canIncrement() {
    // Adjust as required for your new inclusive parameter
    return increment < 0 ?
        count > maximalCount :
        count < maximalCount;
}

This method can be called before any modification of the count member. This prevents successive calls to increment(int) from increasing the count far above the maximum allowed.

This is how broken the counter is:

IntegerSequence.Incrementor inc =
    IntegerSequence.Incrementor
        .create()
        .withIncrement(1 << 30)
        .withStart(0)
        .withMaximalCount(1 << 30)
        .withCallback(i -> {});
for (int i = 0; i < 10; i++) {
    inc.increment(1);
    System.out.println(inc.getCount());
}

prints:

1073741824
-2147483648
-1073741824
0
1073741824
-2147483648
-1073741824
0
1073741824
-2147483648

By changing the count to a long we will not get this behaviour any more. It will just keep incrementing the long. A request for the count using getCount will use the 32-bit truncation of the long, and possibly return 0 or a negative value for a positive integer sequence depending on where the long count has reached.

[aherbert]

This should be documented as returning the current count, or maximalCount if the sequence has exhausted the configured range. This would match the behaviour of the value passed to the callback when the sequence is exhausted.

Then fix the return. Using a 32-bit truncation of a long can return nonsense results (see comment on increment(int).

[aherbert]

Call canIncrement()

[aherbert]

I think we need more validation here. The step should be appropriately signed to match the sign of the difference (max - start). Otherwise you get stupid behaviour:

IntegerSequence.Incrementor inc =
    IntegerSequence.Incrementor
        .create()
        .withStart(0)
        .withMaximalCount(1)
        .withIncrement(-1)
        .withCallback(i -> {});
for (int i = 0; i < 10; i++) {
    inc.increment(1);
    System.out.println(inc.getCount());
}

prints

-1
-2
-3
-4
-5
-6
-7
-8
-9
-10

Without the callback which ignores the sequence exhaustion then this raises an exception immediately as the count is updated to -1 and the check to test for a negative increment if the count is above the maximal count fails.

[aherbert]

This will be negative if (max - start) overflows a positive integer. I think in this case the result should be set at Integer.MAX_VALUE not 0.

[aherbert]

This should be documented to return a positive value in [0, Integer.MAX_VALUE]. It should be noted that the size may be larger than this if the range between the maximalCount and the start cannot be represented as a positive integer.