On Fri, 20 Jan 2023 20:20:32 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>> I tried to clean up the documentation in the `java.awt.color` package and >> specify how the `null` parameters are handled here and there. But it looks >> like the `ICC_profile` class is too specific so I would like to discuss it >> separately. >> >> The ICC_Profile has these methods: >> * `getInstance(int)` - always throws specified >> `IllegalArgumentException` on the wrong constant >> * `getInstance(byte[])` - always throws specified >> `IllegalArgumentException` on `null` >> * `getInstance(String fileName)` - always throws unspecified >> `NullPointerException` on `null` >> * `getInstance(InputStream)` - thrown unspecified >> `NullPointerException` before >> [JDK-8227816](https://github.com/openjdk/jdk/commit/af20c6b9c4a3a21c1e2c7f56721e1077e7d8f388) >> and now throws specified but misleading `IOException: Stream closed` >> exception on `null` >> >> So we have 3 methods that can get null as a parameter and each throw a >> different exception. >> >> Note that all of the specs for the methods above have this part: >> >>> @throws IllegalArgumentException If the stream does not contain valid ICC >>> Profile data >> >> So I have an idea to change `getInstance(String)` and >> `getInstance(InputStream)` to throw `IllegalArgumentException` even if I >> personally prefer NPE to be thrown. >> >> From another point of view the `ICC_Profile` profile has other methods: >> * `write(String fileName)` - always throws unspecified >> `NullPointerException` on `null` >> * `write(OutputStream) ` - always throws unspecified >> `NullPointerException` on `null` >> >> I am not sure we can throw `IllegalArgumentException` from the `write` >> methods, so the specification for `getInstance(String)` and >> `getInstance(InputStream)` could be updated to throw NPE on null same as >> related `write`. >> >> Thoughts? > > Sergey Bylokhov has updated the pull request incrementally with two > additional commits since the last revision: > > - Unify pairs for filenames and streams > - Revert "8299333: Unify used exceptions by all variants of > ICC_Profile.getInstance(null)" > > This reverts commit db5cfdc280c00e38949469f9bc6fcb3d06a12716. I finally got around to reviewing the CSR. You can now finalize it. src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 869: > 867: */ > 868: public static ICC_Profile getInstance(String fileName) throws > IOException { > 869: if (fileName == null) { The reasons given for IOException seems more appropriate since a null file cannot be opened but I'm sure you don't think that is the right exception either I think it might be best to update the spec along with this change to be clear. src/java.desktop/share/classes/java/awt/color/ICC_Profile.java line 902: > 900: */ > 901: public static ICC_Profile getInstance(InputStream s) throws > IOException { > 902: if (s == null) { Same comment here. We should make it clear in the spec. ------------- Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.org/jdk/pull/11784
