On Fri, 20 Feb 2026 16:29:59 GMT, Alexey Ivanov <[email protected]> wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8377568
>
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 544:
>
>> 542: }
>> 543:
>> 544: void checkIndex(int i) {
>
> Should both `checkIndex(int i)` and `checkIndex(int bank, int i)` be declared
> as `final`? Neither is supposed to be overridden.
I don't see it as important but I can do that.
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 545:
>
>> 543:
>> 544: void checkIndex(int i) {
>> 545: if ((i + offset) >= size) {
>
> Is there ever a possibility for integer overflow? Should we care?
I don't think it matters here.
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 554:
>
>> 552: if ((i + offsets[bank]) >= size) {
>> 553: throw new ArrayIndexOutOfBoundsException("Invalid index
>> (bankOffset+i) is " +
>> 554: "(" + offsets[bank] + " + " + i + ") which is too large
>> for size : " + size);
>
> Now, the `if` statement is indented by 2 spaces instead of 4.
oops. fixing.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 93:
>
>> 91: * @param numBanks The number of banks in the {@code DataBuffer}.
>> 92: * @throws IllegalArgumentException if {@code size} is less than or
>> equal to zero.
>> 93: * or {@code numBanks} is less than one.
>
> Suggestion:
>
> * @throws IllegalArgumentException if {@code size} is less than or equal
> to zero,
> * or {@code numBanks} is less than one.
fixed.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 134:
>
>> 132: }
>> 133: if (size <= 0 || size > dataArray.length) {
>> 134: throw new IllegalArgumentException("Bad size : " + size);
>
> Is there a reason for a space *before* the colon?
just the way I think it looks cleaner.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 167:
>
>> 165: throw new NullPointerException("Null dataArray");
>> 166: }
>> 167: if (size <= 0 || (size + offset) > dataArray.length) {
>
> Could it ever happen that `size + offset` overflows such that the condition
> becomes `false` and the parameters are accepted as valid?
I've updated the code to check for that.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 193:
>
>> 191: * @throws IllegalArgumentException if {@code dataArray} does not
>> have at least one bank.
>> 192: * @throws NullPointerException if any bank of {@code dataArray} is
>> {@code null}.
>> 193: * or {@code (offset + size)} is greater than the length of
>> {@code dataArray}
>
> Suggestion:
>
> * @throws IllegalArgumentException if {@code dataArray} does not have at
> least one bank.
> * @throws NullPointerException if any bank of {@code dataArray} is
> {@code null}.
>
> It looks as if a copy-paste mistake as `(offset + size) > dataArray.length`
> doesn't apply.
fixed
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 195:
>
>> 193: * or {@code (offset + size)} is greater than the length of
>> {@code dataArray}
>> 194: * @throws IllegalArgumentException if the length of any bank of
>> {@code dataArray}.
>> 195: * is less than {@code size}.
>
> Suggestion:
>
> * @throws IllegalArgumentException if the length of any bank of {@code
> dataArray}
> * is less than {@code size}.
>
> It's one sentence, isn't it?
removed stray "."
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 244:
>
>> 242: * @throws IllegalArgumentException if {@code dataArray} does not
>> have at least one bank.
>> 243: * @throws NullPointerException if {@code offsets} is {@code null}.
>> 244: * @throws ArrayIndexOutOfBoundsException if the lengths of {@code
>> dataArray} and {@code offsets} differ.
>
> The code throws `IllegalArgumentException` if `dataArray.length >
> offsets.length` instead of AIOOBE.
It should be AIIOBE. This is the one case I see where a DataBuffer superclass
constructor declares and even throws exception and it said AIIOBE so I decided
no reason to change that.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 247:
>
>> 245: * @throws NullPointerException if any bank of {@code dataArray} is
>> {@code null}.
>> 246: * @throws IllegalArgumentException if the length of any bank of
>> {@code dataArray}.
>> 247: * is less than ({@code size} + offsets[bankIndex]).
>
> Suggestion:
>
> * @throws IllegalArgumentException if the length of any bank of {@code
> dataArray}
> * is less than {@code (size + offsets[bankIndex])}.
fixed
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 259:
>
>> 257: if (dataArray.length == 0) {
>> 258: throw new IllegalArgumentException("Must have at least one
>> bank");
>> 259: }
>
> The first three conditions are the same as in the constructor above. Are they
> worth moving into a static helper method?
I've updated all these subclass constructors to use static helpers as much as
possible.
> src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 95:
>
>> 93: if (numBanks < 1) {
>> 94: throw new IllegalArgumentException("Must have at least one
>> bank");
>> 95: }
>
> Why can't these checks be performed in the super class, `DataBuffer`?
>
> I see the same statements here, in `DataBufferDouble`, and in
> `DataBufferByte`:
>
> https://github.com/prrace/jdk/blob/f17761b4b0ced616734825d486d6a3250c81c24e/src/java.desktop/share/classes/java/awt/image/DataBufferByte.java#L95-L102
I could but I don't think there is much to be gained by it. More trouble than
it is worth
Not all checks can be moved there and here I have the checks in the same place
the RTE is declared and thrown.
Also I'd have to update that spec as well as this one. And javadoc doesn't
inherit RTE so they will have to be here anyway .. so all you get is a few
shared lines .. especially since I've now created utility methods.
> src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 126:
>
>> 124: if (dataArray == null) {
>> 125: throw new NullPointerException("Null dataArray");
>> 126: }
>
> Can we drop the explicit `null`-check? If the `dataArray` parameter is
> `null`, getting the length of the array in the following statement will
> result in NPE.
I prefer to keep it so I can give a meaningful exception message. As well as
being explicit.
> src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 129:
>
>> 127: if (size <= 0 || size > dataArray.length) {
>> 128: throw new IllegalArgumentException("Bad size : " + size);
>> 129: }
>
> A static helper method `checkArraySize` would avoid duplicating validation of
> the `dataArray` size between different data types:
>
>
> checkArraySize(size, dataArray.length);
>
>
> where
>
>
> static void checkArraySize(int size, int dataArrayLength) {
> if (size <= 0 || size > dataArrayLength) {
> throw new IllegalArgumentException("Bad size : " + size);
> }
> }
>
>
> is in the `DataBuffer` class.
I added static helpers.
> src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 332:
>
>> 330: */
>> 331: public int getElem(int i) {
>> 332: checkIndex(i);
>
> Shouldn't the `DataBuffer.getElem` method also specify `@throws
> ArrayIndexOutOfBoundsException`?
I don't see a need to touch the super-class spec. The method there doesn't
actually throw anything.
It just calls the abstract method getElem(int, int). So it is all down to the
subclasses.
> src/java.desktop/share/classes/java/awt/image/DataBufferDouble.java line 365:
>
>> 363: */
>> 364: public void setElem(int i, int val) {
>> 365: checkIndex(i);
>
> Shouldn't the `DataBuffer.setElem` method also specify `@throws
> ArrayIndexOutOfBoundsException`?
As mentioned above, this is an abstract method. I didn't see a need to touch it
to declare a RTE
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834894590
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834898507
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834899042
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834906756
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834908941
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834911555
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834932087
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834918421
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2835166664
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834936740
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834939109
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2835184416
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834952287
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834950397
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834946782
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2834949276
