On Wed, 3 Dec 2025 23:11:13 GMT, Phil Race <[email protected]> wrote:
>> Specifying the behaviour of BufferedImage constructors for invalid
>> dimensions is long overdue.
>>
>> The behaviour for image types and sizes <= 0 is unchanged by this PR.
>> Also in many cases the behaviour for sizes that are too large is also
>> unchanged.
>> In some cases, the behaviour is changed from "accidental"
>> NegativeArraySizeException to a consistent IllegalArgumentException.
>>
>> In no case is anything changed that would affect the possibility to
>> construct a BufferedImage.
>>
>> A test is provided to ensure the behaviour.
>>
>> A CSR is provided too : https://bugs.openjdk.org/browse/JDK-8369155
>
> Phil Race has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 4617681
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 297:
> 295: * A {@code DataBuffer} is a container for one or more banks of
> 296: * Java primitive arrays so the number of samples that can be
> 297: * stored are limited by the maximum size of a Java array.
Suggestion:
* A {@code DataBuffer} is a container for one or more banks of
* Java primitive arrays, so the number of samples that can be
* stored is limited by the maximum size of a Java array.
A comma before _“so”_ improves readability by separating the dependent clause.
“*the number* of samples … is,” the verb should be singular, isn't it?
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 300:
> 298: * This is at most {@code Integer.MAX_VALUE}.
> 299: * The number of samples per-pixel for an {@code imageType} affect
> 300: * the maximum. For example, if an image format uses bytes to store
Suggestion:
* The number of samples per-pixel for an {@code imageType} affects
* the maximum. For example, if an image format uses bytes to store
“The number … affects…”?
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 340:
> 338: if (width <= 0 || height <= 0) {
> 339: throw new IllegalArgumentException(
> 340: "width " + width + " height " + height + " must
> both be > 0");
Suggestion:
"width " + width + "and height " + height + " must both be
> 0");
width *and* height?
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 602:
> 600: throw new IllegalArgumentException(
> 601: "width " + width + " height " + height + "
> overflow int");
> 602: }
Does it make sense to move these checks into a separate method to avoid
duplicating code at lines 338–346?
https://github.com/openjdk/jdk/blob/7fb58b903bb747000d40318030db27187de75e98/src/java.desktop/share/classes/java/awt/image/BufferedImage.java#L338-L346
`spp` can be passed as a parameter to the helper method in the above case, or
the value of 1 in this case.
Using one method to validate the parameters will ensure the messages are
consistent. Currently, the asterisk `*` is _still_ missing at line 601 whereas
it's _now_ present at line 345.
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 674:
> 672: * {@code raster} is incompatible with {@code cm}
> 673: * @throws IllegalArgumentException if
> 674: * {@code raster}, {@code minX} or {@code minY} is not zero
Suggestion:
* @throws IllegalArgumentException if
* {@code raster.minX} or {@code raster.minY} is not zero
The code verifies the values of `minX` and `minY` of the `raster`.
I didn't figure out the correct way in [my previous
comment](https://github.com/openjdk/jdk/pull/27640#discussion_r2565722017).
-------------
PR Review: https://git.openjdk.org/jdk/pull/27640#pullrequestreview-3541788311
PR Review Comment: https://git.openjdk.org/jdk/pull/27640#discussion_r2590346664
PR Review Comment: https://git.openjdk.org/jdk/pull/27640#discussion_r2590350836
PR Review Comment: https://git.openjdk.org/jdk/pull/27640#discussion_r2590406998
PR Review Comment: https://git.openjdk.org/jdk/pull/27640#discussion_r2590390135
PR Review Comment: https://git.openjdk.org/jdk/pull/27640#discussion_r2590378771
