aherbert commented on code in PR #335:
URL: https://github.com/apache/commons-math/pull/335#discussion_r4352187441
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -354,7 +378,7 @@ public boolean hasNext() {
@Override
public Integer next() {
if (canIncrement(0)) {
Review Comment:
Call `canIncrement()`
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -276,7 +295,7 @@ public int getMaximalCount() {
* @return the current count.
*/
public int getCount() {
- return count;
+ return (int) count;
Review Comment:
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)`.
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -89,15 +90,16 @@ public static class Range implements Iterable<Integer> {
* @param max Last value of the range that satisfies the above
* construction rule.
* @param step Increment.
+ * @throws ZeroException if {@code step} is zero
*/
public Range(int start,
int max,
int step) {
this.start = start;
this.max = max;
- this.step = step;
+ this.step = Incrementor.checkIncrement(step);
- final int s = (max - start) / step + 1;
+ final int s = (max - start) / this.step + 1;
this.size = s < 0 ? 0 : s;
Review Comment:
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.
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -319,7 +343,7 @@ public void increment(int nTimes) throws
MaxCountExceededException {
throw new NotStrictlyPositiveException(nTimes);
}
- count += nTimes * increment;
+ count += (long) nTimes * increment;
Review Comment:
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:
```java
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:
```java
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.
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -89,15 +90,16 @@ public static class Range implements Iterable<Integer> {
* @param max Last value of the range that satisfies the above
* construction rule.
* @param step Increment.
+ * @throws ZeroException if {@code step} is zero
*/
public Range(int start,
int max,
int step) {
this.start = start;
this.max = max;
- this.step = step;
+ this.step = Incrementor.checkIncrement(step);
Review Comment:
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:
```java
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.
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
