RFR: 8377568: DataBuffer constructors and methods do not specify required exceptions [v5]
Phil Race
prr at openjdk.org
Fri Feb 20 21:40:48 UTC 2026
On Fri, 20 Feb 2026 16:29:59 GMT, Alexey Ivanov <aivanov at openjdk.org> 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
More information about the client-libs-dev
mailing list