renechoi opened a new pull request, #1766:
URL: https://github.com/apache/commons-lang/pull/1766
`Instants.toMillisSince(Instant)` documents two bounds:
> - If the duration milliseconds are greater than `Long.MAX_VALUE`, then
return `Long.MAX_VALUE`.
> - If the duration milliseconds are lesser than `Long.MIN_VALUE`, then
return `Long.MIN_VALUE`.
It selects the bound with `toBound(instant2, Long.MIN_VALUE,
Long.MAX_VALUE)`, which tests the sign of the *instant's* epoch second. The
value being bound is `DurationUtils.since(instant)`, the duration from that
instant to now, and its sign is the opposite one: a past instant yields a
positive duration, a future instant a negative one. The suite already pins that
convention in `testToMillisSincePastInstantIsPositive` and
`testToMillisSinceFutureInstantIsNegative`.
`Duration.toMillis()` only overflows past roughly ±292 million years, so an
instant is either far enough in the past that both the duration and the epoch
second are extreme, or far enough in the future that they are, and the two
always disagree in sign. Every reachable overflow is therefore bound to the
wrong end. Against master (8f8f3b2):
```
Duration.between(Instant.MIN, now).isNegative() = false
(seconds=31557015952864320)
Javadoc says: millis > Long.MAX_VALUE -> 9223372036854775807
actual -> -9223372036854775808
Duration.between(Instant.MAX, now).isNegative() = true
(seconds=-31556888078758080)
Javadoc says: millis < Long.MIN_VALUE -> -9223372036854775808
actual -> 9223372036854775807
```
`toEpochMillis` is unaffected: there the quantity being bound *is* the
instant, so the epoch-second test in `toBound` is the right discriminator. Only
`toMillisSince` reuses it against a quantity of the opposite sign.
The fix keeps the `Duration` and binds on `duration.isNegative()`, which is
the quantity the Javadoc describes. Results that do not overflow are unchanged.
The two existing overflow tests asserted the old behavior, and their Javadoc
says so explicitly ("the bound is `Long.MAX_VALUE` because the instant's epoch
second is positive"), so they are inverted and renamed for the direction they
now cover. Both fail without this change. `Instants` is new in 3.21.0 and not
yet released, so no released behavior changes.
Verified locally on JDK 21 with the default goal (`mvn`): 89186 tests, 0
failures, 0 errors, and rat, checkstyle, japicmp, spotbugs, pmd and javadoc all
clean.
--
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]
