garydgregory commented on code in PR #790:
URL: https://github.com/apache/commons-io/pull/790#discussion_r2395537924
##########
src/main/java/org/apache/commons/io/output/NullWriter.java:
##########
@@ -77,17 +79,24 @@ public Writer append(final CharSequence csq) {
}
/**
- * Does nothing, like writing to {@code /dev/null}.
+ * Does nothing except argument validation, like writing to {@code
/dev/null}.
*
- * @param csq The character sequence to write.
- * @param start The index of the first character to write.
- * @param end The index of the first character to write (exclusive).
- * @return this writer.
+ * @param csq The character sequence from which a subsequence will be
+ * appended.
+ * If {@code csq} is {@code null}, it is treated as if it were
+ * {@code "null"}.
+ * @param start The index of the first character in the subsequence.
+ * @param end The index of the character following the last character in
the
+ * subsequence.
+ * @return {@code this} instance.
+ * @throws IndexOutOfBoundsException If {@code start} or {@code end} are
negative, {@code end} is
+ * greater than {@code csq.length()}, or
{@code start} is greater
+ * than {@code end}.
* @since 2.0
*/
@Override
public Writer append(final CharSequence csq, final int start, final int
end) {
- //to /dev/null
Review Comment:
What about moving this comment down below (instead of deleting it) like in
the other methods with this kind of comment?
##########
src/main/java/org/apache/commons/io/input/ClosedInputStream.java:
##########
@@ -86,6 +86,10 @@ public int read() {
*/
@Override
public int read(final byte[] b, final int off, final int len) throws
IOException {
+ IOUtils.checkFromIndexSize(b, off, len);
Review Comment:
The class and method Javadoc are out of sync with the code now.
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/input/BoundedReader.java:
##########
@@ -121,6 +123,7 @@ public int read() throws IOException {
*/
@Override
public int read(final char[] cbuf, final int off, final int len) throws
IOException {
+ IOUtils.checkFromIndexSize(cbuf, off, len);
Review Comment:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/input/CharSequenceReader.java:
##########
@@ -209,14 +210,13 @@ public int read() {
*/
@Override
public int read(final char[] array, final int offset, final int length) {
+ IOUtils.checkFromIndexSize(array, offset, length);
Review Comment:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/output/WriterOutputStream.java:
##########
@@ -448,6 +448,7 @@ public void write(final byte[] b) throws IOException {
*/
@Override
public void write(final byte[] b, int off, int len) throws IOException {
+ IOUtils.checkFromIndexSize(b, off, len);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/output/ChunkedOutputStream.java:
##########
@@ -172,6 +172,7 @@ int getChunkSize() {
*/
@Override
public void write(final byte[] data, final int srcOffset, final int
length) throws IOException {
+ IOUtils.checkFromIndexSize(data, srcOffset, length);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/IOUtils.java:
##########
@@ -406,6 +406,183 @@ private static char[] charArray(final int size) {
return new char[size];
}
+ /**
+ * Validates that the sub-range {@code [off, off + len)} is within the
bounds of the given array.
+ *
+ * <p>The range is valid if all of the following hold:</p>
+ * <ul>
+ * <li>{@code off >= 0}</li>
+ * <li>{@code len >= 0}</li>
+ * <li>{@code off + len <= array.length}</li>
+ * </ul>
+ *
+ * <p>If the range is invalid, throws {@link IndexOutOfBoundsException}
with a descriptive message.</p>
+ *
+ * <p>Typical usage in {@link InputStream#read(byte[], int, int)} and
{@link OutputStream#write(byte[], int, int)} implementations:</p>
+ *
+ * <pre><code>
+ * public int read(byte[] b, int off, int len) throws IOException {
+ * IOUtils.checkFromIndexSize(b, off, len);
+ * if (len == 0) {
+ * return 0;
+ * }
+ * ensureOpen();
+ * // perform read...
+ * }
+ *
+ * public void write(byte[] b, int off, int len) throws IOException {
+ * IOUtils.checkFromIndexSize(b, off, len);
+ * if (len == 0) {
+ * return;
+ * }
+ * ensureOpen();
+ * // perform write...
+ * }
+ * </code></pre>
+ *
+ * @param array the array against which the range is validated
+ * @param off the starting offset into the array (inclusive)
+ * @param len the number of elements to access
+ * @throws NullPointerException if {@code array} is {@code null}
+ * @throws IndexOutOfBoundsException if the range {@code [off, off + len)}
is out of bounds for {@code array}
+ * @see InputStream#read(byte[], int, int)
+ * @see OutputStream#write(byte[], int, int)
+ * @since 2.21.0
+ */
+ public static void checkFromIndexSize(final byte[] array, final int off,
final int len) {
+ checkFromIndexSize(off, len, Objects.requireNonNull(array, "byte
array").length);
+ }
+
+ /**
+ * Validates that the sub-range {@code [off, off + len)} is within the
bounds of the given array.
+ *
+ * <p>The range is valid if all of the following hold:</p>
+ * <ul>
+ * <li>{@code off >= 0}</li>
+ * <li>{@code len >= 0}</li>
+ * <li>{@code off + len <= array.length}</li>
+ * </ul>
+ *
+ * <p>If the range is invalid, throws {@link IndexOutOfBoundsException}
with a descriptive message.</p>
+ *
+ * <p>Typical usage in {@link Reader#read(char[], int, int)} and {@link
Writer#write(char[], int, int)} implementations:</p>
+ *
+ * <pre><code>
+ * public int read(char[] cbuf, int off, int len) throws IOException {
+ * ensureOpen();
+ * IOUtils.checkFromIndexSize(cbuf, off, len);
+ * if (len == 0) {
+ * return 0;
+ * }
+ * // perform read...
+ * }
+ *
+ * public void write(char[] cbuf, int off, int len) throws IOException {
+ * ensureOpen();
+ * IOUtils.checkFromIndexSize(cbuf, off, len);
+ * if (len == 0) {
+ * return;
+ * }
+ * // perform write...
+ * }
+ * </code></pre>
+ *
+ * @param array the array against which the range is validated
+ * @param off the starting offset into the array (inclusive)
+ * @param len the number of characters to access
+ * @throws NullPointerException if {@code array} is {@code null}
+ * @throws IndexOutOfBoundsException if the range {@code [off, off + len)}
is out of bounds for {@code array}
+ * @see Reader#read(char[], int, int)
+ * @see Writer#write(char[], int, int)
+ * @since 2.21.0
+ */
+ public static void checkFromIndexSize(final char[] array, final int off,
final int len) {
+ checkFromIndexSize(off, len, Objects.requireNonNull(array, "char
array").length);
+ }
+
+ /**
+ * Validates that the sub-range {@code [off, off + len)} is within the
bounds of the given string.
+ *
+ * <p>The range is valid if all of the following hold:</p>
+ * <ul>
+ * <li>{@code off >= 0}</li>
+ * <li>{@code len >= 0}</li>
+ * <li>{@code off + len <= str.length()}</li>
+ * </ul>
+ *
+ * <p>If the range is invalid, throws {@link IndexOutOfBoundsException}
with a descriptive message.</p>
+ *
+ * <p>Typical usage in {@link Writer#write(String, int, int)}
implementations:</p>
+ *
+ * <pre><code>
+ * public void write(String str, int off, int len) throws IOException {
+ * IOUtils.checkFromIndexSize(str, off, len);
+ * if (len == 0) {
+ * return;
+ * }
+ * // perform write...
+ * }
+ * </code></pre>
+ *
+ * @param str the string against which the range is validated
+ * @param off the starting offset into the string (inclusive)
+ * @param len the number of characters to write
+ * @throws NullPointerException if {@code str} is {@code null}
+ * @throws IndexOutOfBoundsException if the range {@code [off, off + len)}
is out of bounds for {@code str}
+ * @see Writer#write(String, int, int)
+ * @since 2.21.0
+ */
+ public static void checkFromIndexSize(final String str, final int off,
final int len) {
+ checkFromIndexSize(off, len, Objects.requireNonNull(str,
"str").length());
+ }
+
+ static void checkFromIndexSize(final int off, final int len, final int
arrayLength) {
+ if ((off | len | arrayLength) < 0 || arrayLength - len < off) {
+ throw new IndexOutOfBoundsException(String.format("Range [%s, %<s
+ %s) out of bounds for length %s", off, len, arrayLength));
+ }
+ }
+
+ /**
+ * Validates that the sub-sequence {@code [fromIndex, toIndex)} is within
the bounds of the given {@link CharSequence}.
+ *
+ * <p>The sub-sequence is valid if all of the following hold:</p>
+ * <ul>
+ * <li>{@code fromIndex >= 0}</li>
+ * <li>{@code fromIndex <= toIndex}</li>
+ * <li>{@code toIndex <= seq.length()}</li>
+ * </ul>
+ *
+ * <p>If {@code seq} is {@code null}, it is treated as the literal string
{@code "null"} (length {@code 4}).</p>
+ *
+ * <p>If the range is invalid, throws {@link IndexOutOfBoundsException}
with a descriptive message.</p>
+ *
+ * <p>Typical usage in {@link Appendable#append(CharSequence, int, int)}
implementations:</p>
+ *
+ * <pre><code>
+ * public Appendable append(CharSequence csq, int start, int end) throws
IOException {
+ * IOUtils.checkFromToIndex(csq, start, end);
+ * // perform append...
+ * return this;
+ * }
+ * </code></pre>
+ *
+ * @param seq the character sequence to validate (may be {@code
null}, treated as {@code "null"})
+ * @param fromIndex the starting index (inclusive)
+ * @param toIndex the ending index (exclusive)
+ * @throws IndexOutOfBoundsException if the range {@code [fromIndex,
toIndex)} is out of bounds for {@code seq}
+ * @see Appendable#append(CharSequence, int, int)
+ * @since 2.21.0
+ */
+ public static void checkFromToIndex(final CharSequence seq, final int
fromIndex, final int toIndex) {
+ checkFromToIndex(fromIndex, toIndex, seq != null ? seq.length() : 4);
+ }
+
+ static void checkFromToIndex(final int fromIndex, final int toIndex, final
int length) {
+ if (fromIndex < 0 | toIndex < fromIndex | length < toIndex) {
Review Comment:
Using `|` instead of `||` evaluates everything unneccessarily, see the JRE
implementation (Java 16-25's
`jdk.internal.util.Preconditions.checkFromToIndex(long, long, long,
BiFunction<String, List<Number>, X>)`)
##########
src/main/java/org/apache/commons/io/output/ChunkedWriter.java:
##########
@@ -74,6 +74,7 @@ public ChunkedWriter(final Writer writer, final int
chunkSize) {
*/
@Override
public void write(final char[] data, final int srcOffset, final int
length) throws IOException {
+ IOUtils.checkFromIndexSize(data, srcOffset, length);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/input/BOMInputStream.java:
##########
@@ -444,6 +444,10 @@ public int read(final byte[] buf) throws IOException {
*/
@Override
public int read(final byte[] buf, int off, int len) throws IOException {
+ IOUtils.checkFromIndexSize(buf, off, len);
Review Comment:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/input/SequenceReader.java:
##########
@@ -114,9 +115,9 @@ public int read() throws IOException {
*/
@Override
public int read(final char[] cbuf, int off, int len) throws IOException {
- Objects.requireNonNull(cbuf, "cbuf");
- if (len < 0 || off < 0 || off + len > cbuf.length) {
- throw new IndexOutOfBoundsException("Array Size=" + cbuf.length +
", offset=" + off + ", length=" + len);
+ IOUtils.checkFromIndexSize(cbuf, off, len);
Review Comment:
Either we should inherit the whole Javadoc or add a real Javadoc to document
`@throws` `IndexOutOfBoundsException`.
##########
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:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/input/ReaderInputStream.java:
##########
@@ -456,14 +455,11 @@ public int read(final byte[] b) throws IOException {
*/
@Override
public int read(final byte[] array, int off, int len) throws IOException {
- Objects.requireNonNull(array, "array");
- if (len < 0 || off < 0 || off + len > array.length) {
- throw new IndexOutOfBoundsException("Array size=" + array.length +
", offset=" + off + ", length=" + len);
- }
- int read = 0;
+ IOUtils.checkFromIndexSize(array, off, len);
Review Comment:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/output/AppendableWriter.java:
##########
@@ -124,10 +126,7 @@ public T getAppendable() {
*/
@Override
public void write(final char[] cbuf, final int off, final int len) throws
IOException {
- Objects.requireNonNull(cbuf, "cbuf");
- if (len < 0 || off + len > cbuf.length) {
- throw new IndexOutOfBoundsException("Array Size=" + cbuf.length +
", offset=" + off + ", length=" + len);
- }
+ IOUtils.checkFromIndexSize(cbuf, off, len);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
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);
Review Comment:
k, I am resolving these types of issues (GitHub PR resolving that is).
But:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
src/main/java/org/apache/commons/io/output/ClosedOutputStream.java:
##########
@@ -73,6 +75,7 @@ public void flush() throws IOException {
*/
@Override
public void write(final byte b[], final int off, final int len) throws
IOException {
+ IOUtils.checkFromIndexSize(b, off, len);
Review Comment:
OK.
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/input/NullReader.java:
##########
@@ -276,6 +278,10 @@ public int read(final char[] chars) throws IOException {
*/
@Override
public int read(final char[] chars, final int offset, final int length)
throws IOException {
+ IOUtils.checkFromIndexSize(chars, offset, length);
Review Comment:
Javadoc `@throws` IndexOutOfBoundsException is missing.
##########
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);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
src/main/java/org/apache/commons/io/output/ThresholdingOutputStream.java:
##########
@@ -249,6 +250,7 @@ public void write(final byte[] b) throws IOException {
@SuppressWarnings("resource") // the underlying stream is managed by a
subclass.
@Override
public void write(final byte[] b, final int off, final int len) throws
IOException {
+ IOUtils.checkFromIndexSize(b, off, len);
Review Comment:
Javadoc `@throws IndexOutOfBoundsException ...` is missing.
##########
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:
I'd think maintaining the behavior is simpler than creating a possible
surprise for downstream users. We can always review later. The point of this PR
is only to use the new API.
--
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]
