Copilot commented on code in PR #790:
URL: https://github.com/apache/commons-io/pull/790#discussion_r2394019524
##########
src/main/java/org/apache/commons/io/input/NullInputStream.java:
##########
@@ -292,7 +294,8 @@ public int read(final byte[] bytes) throws IOException {
*/
@Override
public int read(final byte[] bytes, final int offset, final int length)
throws IOException {
- if (bytes.length == 0 || length == 0) {
+ IOUtils.checkFromIndexSize(bytes, offset, length);
+ if (length == 0) {
Review Comment:
The original logic `if (bytes.length == 0 || length == 0)` handled the case
where the byte array itself was empty. The new logic only checks if `length ==
0`, potentially changing behavior for empty arrays.
```suggestion
if (bytes.length == 0 || length == 0) {
```
##########
src/main/java/org/apache/commons/io/output/StringBuilderWriter.java:
##########
@@ -149,9 +151,8 @@ public String toString() {
*/
@Override
public void write(final char[] value, final int offset, final int length) {
- if (value != null) {
- builder.append(value, offset, length);
- }
+ IOUtils.checkFromIndexSize(value, offset, length);
+ builder.append(value, offset, length);
Review Comment:
The removed null check for `value` means the method will now append to the
StringBuilder even when `value` is null, which changes the existing behavior.
The original code only appended when `value != null`.
```suggestion
if (value != null) {
IOUtils.checkFromIndexSize(value, offset, length);
builder.append(value, offset, length);
}
```
##########
src/main/java/org/apache/commons/io/output/XmlStreamWriter.java:
##########
@@ -306,6 +306,7 @@ public String getEncoding() {
*/
@Override
public void write(final char[] cbuf, final int off, final int len) throws
IOException {
+ IOUtils.checkFromIndexSize(cbuf, off, len);
Review Comment:
The validation should occur before any other operations. Currently, this
validation comes after the null check for `prologWriter` which means encoding
detection could be performed with invalid parameters.
##########
src/main/java/org/apache/commons/io/input/buffer/CircularBufferInputStream.java:
##########
@@ -125,12 +125,9 @@ public int read() throws IOException {
@Override
public int read(final byte[] targetBuffer, final int offset, final int
length) throws IOException {
- Objects.requireNonNull(targetBuffer, "targetBuffer");
- if (offset < 0) {
- throw new IllegalArgumentException("Offset must not be negative");
- }
- if (length < 0) {
- throw new IllegalArgumentException("Length must not be negative");
+ IOUtils.checkFromIndexSize(targetBuffer, offset, length);
Review Comment:
[nitpick] The removed custom validation logic had more specific error
messages ('Offset must not be negative', 'Length must not be negative') which
may be more helpful for debugging than the generic IndexOutOfBoundsException
message.
```suggestion
Objects.requireNonNull(targetBuffer, "targetBuffer");
if (offset < 0) {
throw new IndexOutOfBoundsException("Offset must not be
negative: " + offset);
}
if (length < 0) {
throw new IndexOutOfBoundsException("Length must not be
negative: " + length);
}
if (offset + length > targetBuffer.length) {
throw new IndexOutOfBoundsException("Offset + length (" +
(offset + length) + ") exceeds buffer length (" + targetBuffer.length + ")");
}
```
--
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]
