On Tue, 5 May 2026 04:16:14 GMT, Prasanta Sadhukhan <[email protected]> 
wrote:

>> Phil Race has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8378464
>
> src/java.desktop/share/classes/java/awt/image/PixelInterleavedSampleModel.java
>  line 82:
> 
>> 80:      *         one of the supported data types
>> 81:      * @throws NullPointerException if {@code bandOffsets} is {@code 
>> null}
>> 82:      * @throws IllegalArgumentException if {@code bandOffsets.length} is >> 0
> 
> It is mentioned
> 
> if (numBands <= 0) {
>             throw new IllegalArgumentException("Number of bands must be > 0");
>         }
> 
> so maybe `@throws IllegalArgumentException if {@code bandOffsets.length} is 
> not greater than 0` is more apt
> but I see ComponentSampleModel the superclass of this, also mentioned the 
> same although its superclass SampleModel mentioned
> `@throws IllegalArgumentException if {@code numBands} is less than 1`

I suspect we have many variations on this across the API. I'm not going to go 
make them all consistent, at least not today, but I'm happy enough with the 
current phrasing because I'm preferring throws clauses which say when an 
exception will be thrown rather than when it will not be thrown.

> src/java.desktop/share/classes/java/awt/image/PixelInterleavedSampleModel.java
>  line 161:
> 
>> 159:      * @throws IllegalArgumentException if the number of bands is not 
>> greater than 0
>> 160:      * @throws RasterFormatException if the number of bands is greater 
>> than
>> 161:      *                               the number of banks in this sample 
>> model.
> 
> Is it "number of banks"? Below it is checking for `bandOffsets.length` so 
> shouldn't it be
> `@throws RasterFormatException if the number of bands is greater than {@code 
> bandOffsets.length}`

Probably it should be. I'll change it.
I remember squinting at that when I copied it from the super-class, where there 
is assumed to be a bank per band. Which is not the case here. There's only one 
bank.
There is still a bankIndex per band, they just all point to bank 0.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/30902#discussion_r3191633202
PR Review Comment: https://git.openjdk.org/jdk/pull/30902#discussion_r3191599573

Reply via email to