On Fri, 20 Mar 2026 22:42:34 GMT, Phil Race <[email protected]> wrote:
>> This fix updates DataBuffer subclasses to actually adhere to their stated >> specifications by rejecting certain invalid parameters for constructors and >> getters and setters. >> A new egression test for each of the constructor and getter/setter cases is >> supplied. >> >> No existing regression tests fail with this change, and standard demos work. >> >> Problems caused by these changes are most likely to occur if the client has >> a bug such that >> - a client uses the constructors that accept an array and then supplies a >> "size" that is greater than the array. >> - a client uses the constructors that accept an array and then supplies a >> "size" that is less than the array and then uses getter/setters that are >> within the array but outside the range specified by size. >> >> Since very few clients (and just one case in the JDK that I found) even use >> these array constructors the changes are unlikely to make a difference to >> clients. >> >> The CSR is ready for review https://bugs.openjdk.org/browse/JDK-8378116 > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8377568 Changes requested by aivanov (Reviewer). 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. 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? 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? src/java.desktop/share/classes/java/awt/image/DataBufferByte.java line 40: > 38: import java.util.Objects; > 39: import static sun.java2d.StateTrackable.State.STABLE; > 40: import static sun.java2d.StateTrackable.State.UNTRACKABLE; Suggestion: import java.util.Objects; import static sun.java2d.StateTrackable.State.STABLE; import static sun.java2d.StateTrackable.State.UNTRACKABLE; You didn't add a blank line between regular import and static imports. There's no way to un-resolve a comment. Look at `Robot.java`, `Component.java`, `Font.java`—all these files have a blank line between regular class imports and static imports. And `DataBuffer.java` follows the convention. 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. 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. ------------- PR Review: https://git.openjdk.org/jdk/pull/29766#pullrequestreview-3992600102 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2975777453 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2975790229 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2976042394 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977297090 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977241974 PR Review Comment: https://git.openjdk.org/jdk/pull/29766#discussion_r2977255519
