On Tue, 3 Mar 2026 19:12:26 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 559:
>
>> 557:
>> 558: // Checks used by subclass constructors.
>> 559: static final void checkSize(int size) {
>
> Do we really need the `final` modifier for a static method? Static methods
> aren't inherited.
It doesn't cause any problems does it ?
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 579:
>
>> 577: static final void checkArraySize(int size, int offset, int
>> arrayLen) {
>> 578: if (size <= 0 || (size + offset) > arrayLen ||
>> 579: (offset > 0) && ((size + offset) < size)) {
>
> Should the condition `(size + offset) > arrayLen` also ensure `offset > 0`?
>
> Suggestion:
>
> if (size <= 0 || offset < 0 || (size + offset) > arrayLen ||
> (size + offset) < size) {
No. offset is allowed to be negative.
> src/java.desktop/share/classes/java/awt/image/DataBuffer.java line 587:
>
>> 585: static final void checkBankSize(int bank, int size, int offset, int
>> arrayLen) {
>> 586: if ((arrayLen < (size + offset)) ||
>> 587: ((offset > 0) && ((size + offset) < size))) {
>
> If `offset` is negative, the condition `(arrayLen < (size + offset))` will
> give unexpected result.
Given that offset is allowed to be negative, what do you mean ?
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 75:
>
>> 73: *
>> 74: * @param size The size of the {@code DataBuffer}.
>> 75: * @throws IllegalArgumentException if {@code size} is less than or
>> equal to zero.
>
> Why doesn't `DataBuffer(int size)` declare the same
> `IllegalArgumentException`?
>
> It would be great to use `{@inheritDoc}` to pull in the description from
> super constructors, but it doesn't work.
There is no such constructor. Probably you meant one of the others. DataBuffer
is abstract so I left it alone.
And I prefer the exception in the same place as the check.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 79:
>
>> 77: public DataBufferByte(int size) {
>> 78: super(STABLE, TYPE_BYTE, size);
>> 79: checkSize(size);
>
> Why not move the call to `checkSize` into the superclass constructor of
> `DataBuffer`?
>
> Any data buffer of `size <= 0` doesn't make sense, then you won't have to
> call `checkSize` explicitly in all the classes.
Same reason as above. So I want it here.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 97:
>
>> 95: super(STABLE, TYPE_BYTE, size, numBanks);
>> 96: checkSize(size);
>> 97: checkNumBanks(numBanks);
>
> The same comment: both `checkSize` and `checkNumBanks` could be called in the
> superclass constructor and will handle all the data types automatically.
I want them here. Not the abstract superclass.
Because I need to document the exceptions here, so I want the checks explicitly
here.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 188:
>
>> 186: checkNumBanks(dataArray.length);
>> 187: for (int b = 0; b < dataArray.length; b++) {
>> 188: Objects.requireNonNull(dataArray, "bank must not be null");
>
> Suggestion:
>
> Objects.requireNonNull(dataArray[b], "bank must not be null");
>
> Does it make sense to indicate the index of the bank in the error message?
fixed.
regarding the error message, I decided against that because it would force
unnecessary string construction at runtime in order to pass it to this method,
even for the 99.9999% case of it not being needed.
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 220:
>
>> 218: * @throws NullPointerException if any bank of {@code dataArray} is
>> {@code null}.
>> 219: * @throws IllegalArgumentException if the length of any bank of
>> {@code dataArray}
>> 220: * is less than ({@code size} + offsets[bankIndex]).
>
> Suggestion:
>
> * is less than {@code (size + offsets[bankIndex])}.
fixed
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 232:
>
>> 230: }
>> 231: for (int b = 0; b < dataArray.length; b++) {
>> 232: Objects.requireNonNull(dataArray, "bank must not be null");
>
> Suggestion:
>
> Objects.requireNonNull(dataArray[b], "bank must not be null");
fixed
> src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 297:
>
>> 295: public int getElem(int i) {
>> 296: checkIndex(i);
>> 297: return (int)(data[i+offset]) & 0xff;
>
> It will thrown AIOOBE without explicit `checkIndex`, you want a custom error
> message, right?
yes
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880859578
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880860735
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880869881
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880883123
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880884158
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880885151
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880889194
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880919595
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880947058
PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2880948625
