[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: RFR: 8299333: Unify exceptions used by all variants of ICC_Profile.getInstance(null) [v2]
From: Phil Race <prr () openjdk ! org>
Date: 2023-02-23 23:49:07
Message-ID: c1NTxXOmD40aWToTZ6YSJKQK2uQkm_hq7S4otTX7S1I=.ca969158-a7dc-44e0-a861-2f6920f646d2 () github ! com
[Download RAW message or body]
On Fri, 20 Jan 2023 20:20:32 GMT, Sergey Bylokhov <serb@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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic