simpleqt commented on PR #923:
URL: https://github.com/apache/commons-io/pull/923#issuecomment-6583170461
You are right, and the exception had two parts. Here is what I found, with
both shapes reproduced against `master` and the head:
* On `master`, a count larger than the remaining bytes throws
`java.lang.ArithmeticException: integer overflow` from `Math.toIntExact` - and
that one is undocumented, as is the negative return (`skip(5)` past EOF
followed by `skip(1)` gives -2), which `InputStream.skip` implicitly forbids by
defining the return as the actual number of bytes skipped.
* On this head, large counts clamp as intended and no input throws that did
not already throw. The only exception left is `skip(negative)` ->
`IllegalArgumentException: Skipping backward is not supported`, which is
deliberate but had no Javadoc at all.
So rather than only documenting it, `7b0803a6c` does both: `skip()` now
carries Javadoc with `@param`, `@return` and `@throws IllegalArgumentException
if n is negative`, plus a note on how this differs from `ByteArrayInputStream`,
and the `Math.addExact` / `Math.toIntExact` calls are gone (`offset += (int)
actualSkip`), which removes the `ArithmeticException` path entirely and
restores the pre-2020 behaviour; the overflow came in with the CodeQL fix in
`aafcd73bc`.
Contract sources used: `InputStream.skip` ("The actual number of bytes
skipped is returned", and "subclasses may handle the negative value
differently"), your own `IOUtils.skip` and `UnsynchronizedReader.skip`, which
document exactly that `IllegalArgumentException` and are pinned by
`testInvalidSkipNUnder` since 2020.
Evidence: with the base revision of the class and the new tests, 19 run, 1
failure, 1 error (`expected <0> but was <-996>`, and the
`ArithmeticException`); with the fix, 19 run, 0 failures, build success. The
full suite on this head is 7,303 tests with only the pre-existing environment
failures (symlink privileges, DNS, GBK locale) and no `ArithmeticException`.
`checkstyle:check`, `apache-rat:check` and `javadoc -Xdoclint:all` are clean.
One open question I deliberately did not decide for you:
`ByteArrayInputStream` returns 0 for `skip(-1)` rather than throwing. Keeping
the documented `IllegalArgumentException` matches this class's history and
`IOUtils.skip`, but if you would prefer parity with `ByteArrayInputStream`,
that is a one-line change on top of this commit.
--
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]
