On Wed, 12 Nov 2025 02:54:08 GMT, Sergey Bylokhov <[email protected]> wrote:
> This is an update to the patch integrated in > https://github.com/openjdk/jdk/pull/28127. It changes the exception from > IllegalArgumentException back to NullPointerException, which was thrown > previously. > > Rationale: > - Since an NPE was already thrown before, this does not introduce a behavior > change > - Throwing NPE on null is the common pattern used across the codebase, > including in the affected package > > Additional notes: > The patch uses the idiomatic way to check parameters for null via > Objects.requireNonNull(). I am not sure whether this code path is performance > critical. The use of >> 1 instead of / 2 suggests that performance might > matter. If this code is performance sensitive, we can remove the > Objects.requireNonNull() call, because even without it the resulting NPE > would still clearly show which variable was null when its field was accessed. > > It is also possible to remove the use of multiplyExact(). We can simply > multiply width/height as long values and compare the result with the data > length. In case of overflow, the existing “Data array too small” exception > would still be thrown. Looks good except for a minor comment in the test. test/jdk/java/awt/image/ConvolveOp/KernelInitialisationTest.java line 49: > 47: System.err.println("Expected: " + expected); > 48: System.err.println("Actual: " + actual); > 49: throw new RuntimeException("Test failed"); Does it make sense to include the class names in the error message too? It helps analysing the failure, you know the reason without opening the log file. ------------- Marked as reviewed by aivanov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/28249#pullrequestreview-3504115865 PR Review Comment: https://git.openjdk.org/jdk/pull/28249#discussion_r2559237017
