On Mon, 23 Mar 2026 15:25:47 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 575:
>
>> 573: // the offset for each bank against the size.
>> 574: if (i >= size) {
>> 575: throw new ArrayIndexOutOfBoundsException("Invalid index
>> (offsets[" + bank + "] +i) is " +
>
> Suggestion:
>
> throw new ArrayIndexOutOfBoundsException("Invalid index
> (offsets[" + bank + "]+i) is " +
>
> No space before `+` for consistency with the above message.
ok
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 581:
>
>> 579:
>> 580: // Checks used by subclass constructors.
>> 581: static void checkSize(int size) {
>
> Suggestion:
>
> // Checks used by subclass constructors.
>
> static void checkSize(int size) {
>
>
> Put a blank line to clearly mark the comment applies to all the following
> methods?
ok
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 605:
>
>> 603: ((offset + size) <= 0) ||
>> 604: ((offset + size) > arrayLen) ||
>> 605: ((offset > 0) && ((offset + size ) < size));
>
> Now that there's `(offset < 0)` in the list, is `(offset > 0) &&` still
> needed in the last condition? I'm sure it's redundant now.
>
> Is the entire last condition needed now?
the previous check was for offset < 0.
This one is offset > 0 .. so if it is == 0 we skip but if it is > 0 we want to
check for overflow.
Possibly the 3rd condition is the one that is obsolete but I decided to keep it.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 227:
>
>> 225: checkSize(size);
>> 226: checkNumBanks(dataArray.length);
>> 227: Objects.requireNonNull(offsets, "offsets must not be null");
>
> The explicit non-null check for `offsets` is redundant, the super constructor
> will throw `NullPointerException` if `offsets` is `null`.
>
> https://github.com/openjdk/jdk/blob/f1169bfcf1e5c916c12e33539a2ba8624eca1ead/src/java.desktop/share/classes/java/awt/image/DataBuffer.java#L273-L276
>
> The code below accesses the first element of the `offsets` array and clones
> it.
ok
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 230:
>
>> 228: if (dataArray.length != offsets.length) {
>> 229: throw new ArrayIndexOutOfBoundsException("Must be an
>> offsets entry for every bank");
>> 230: }
>
> This condition `dataArray.length != offsets.length` is already verified by
> the super constructor per the above comment:
>
> if (numBanks != offsets.length)
>
> where `numBanks` is `dataArray.length` in the call to the super constructor.
ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977805003
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977807009
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977829260
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977870480
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977871063
