simpleqt commented on PR #225:
URL: https://github.com/apache/commons-numbers/pull/225#issuecomment-6641968455
The proposed check works on the values that reach it, and I have it in and
verified on head `a881e482`.
Two things I checked before trusting it on this domain, because the check is
not equivalent to `Math.addExact` in general:
1. It is sound, not just a coincidence on the tests. `((sum ^ a1) & (sum ^
b)) >= 0` requires the sum to agree in sign with `a1` and with `b`
independently, so it fires when the two-step sequence `a1 + b` then `+ a2`
overflows. That is exactly the overflow `Math.addExact` reports, provided `a1 +
b` does not overflow on its own. It cannot: `|a1|` is at most 2^62 because `a1
= (long)(x * 0.5)` with an extra halving above 2^53, and `b` is the low part,
which is about ulp(x)/2. So the pairwise test decides the same question.
2. Outside that domain it disagrees, which is worth knowing for the comment
rather than for this method. `a1 = b = Long.MIN_VALUE, a2 = -1`: the sum is -1,
`sum ^ a1` is 0 because `a1 + b` wraps exactly to 0, so it reports no overflow
while `Math.addExact` throws. The invariant above is what rules that out here.
Verification, all on `a881e482` with the check as you wrote it:
- `mvn -B test -pl commons-numbers-core -Dtest=DDTest` -> `Tests run: 17935,
Failures: 0, Errors: 0, Skipped: 2`, BUILD SUCCESS. Note that this includes the
parameterized `testLongSum` cases, so the overflow paths in that suite all
still behave.
- I also ran a probe over the boundary triples of the form `a1, a2, b in
{MIN, MIN+1, MIN+2, -2, -1, 0, 1, 2, MAX-2, MAX-1, MAX, ±Integer.MIN/MAX,
±2^62, ±1234567890123456789}` to see where the two checks disagree; the
disagreements are all cases where `a1 + b` overflows by itself, i.e. outside
this method's domain. Happy to paste the probe if useful, it is 40 lines.
On `|| sum == 0` in my version: it is redundant with the new check, not with
the old one. The pairing shows it — `(sum ^ a) >= 0 && sum != 0` holds exactly
when the sum equals `a1 + b` and is non-zero, and the pairwise test holds
exactly when the sum agrees in sign with both `a1` and `b`; on this domain
those are the same predicate. So dropping it is right.
The test method is moved above `testLongSum` as you asked. I have left the
106-bit strengthening of `testLongSum` to you as you said, and I will re-run
whatever you add once it is in.
--
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]
