-
Notifications
You must be signed in to change notification settings - Fork 6.3k
4617681: constructor of BufferedImage throws unexpected IllegalArgumentException #27640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back prr! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
* not greater than zero. | ||
* @throws IllegalArgumentException if the multiplication product of | ||
* {@code width}, {@code height}, and the number of samples per pixel | ||
* for the specified format exceeds the maximum length of a Java array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check seems too strict. It is possible to implement a BufferedImage that splits its internal data into multiple surfaces/arrays. It might be better to phrase this as optional:
“for the specified format exceeds the maximum supported length”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had looked at using a BandedRaster but there were a decent number of places that relied on the current format, even inside the JDK.
The work and compatibility concerns to address these are out of all proportion to simply documenting what has been true since the very beginning - 27 years plus - without a compelling reason.
The specification has the word "may" so it is not disallowed, but I think it would be risky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work and compatibility concerns to address these are out of all proportion to simply documenting what has been true since the very beginning - 27 years plus - without a compelling reason.
What compatibility impact? It has been throwing exceptions all this time, but currently it is allowed to work if we change that in the future, unlike the current proposal, which describes all implementation details as part of the specification, making it much harder to change later.
Instead we can mention "maximum supported length" in the spec and in the "impl section" mention that currently it is the size of the array/maxint,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work and compatibility concerns to address these are out of all proportion to simply documenting what has been true since the very beginning - 27 years plus - without a compelling reason.
What compatibility impact?
That is in reponse to your suggestion to change the internal raster type. Not to a spec working change.
It has been throwing exceptions all this time, but currently it is allowed to work if we change that in the future, unlike the current proposal, which describes all implementation details as part of the specification, making it much harder to change later.
(1) No it allows leeway
(2) Even if it didn't we can relax it if there's ever a compelling enough reason - which I doubt we'll find.
Instead we can mention "maximum supported length" in the spec and in the "impl section" mention that currently it is the size of the array/maxint,
I don't see how that is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how that is better.
The specification will include a note about certain limitations, and under @implNote, we will describe the actual implementation-specific limitation. I believe that tag is appropriate in this context:
"@implNote" — Implementation Notes. This section contains informative notes about the implementation, such as advice for implementors or performance characteristics specific to this class in this version of the JDK. The information in this section may change from release to release. These characteristics may also vary across platforms, vendors, and JDK versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a statement of fact. Not an implNote.
If someone used different storage the clause would still be accurate even if there were no actual examples of it left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone used different storage the clause would still be accurate even if there were no actual examples of it left.
I still would like to clarify, how the next statement could be correct if another storage is in use?
IllegalArgumentException if the multiplication product of
* {@code width}, {@code height}, and the number of samples per pixel
* for the specified format exceeds the maximum length of a Java array.
It is strictly required to throw this exception here and there, even if we will split the data across two arrays.
Specifying the behaviour of BufferedImage constructors for invalid dimensions is long overdue.
The behaviour for image types and sizes <= 0 is unchanged by this PR.
Also in many cases the behaviour for sizes that are too large is also unchanged.
In some cases, the behaviour is changed from "accidental" NegativeArraySizeException to a consistent IllegalArgumentException.
In no case is anything changed that would affect the possibility to construct a BufferedImage.
A test is provided to ensure the behaviour.
A CSR is provided too : https://bugs.openjdk.org/browse/JDK-8369155
Progress
Warning
4617681: constructor of BufferedImage throws unexpected IllegalArgumentException
Issues
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27640/head:pull/27640
$ git checkout pull/27640
Update a local copy of the PR:
$ git checkout pull/27640
$ git pull https://git.openjdk.org/jdk.git pull/27640/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27640
View PR using the GUI difftool:
$ git pr show -t 27640
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27640.diff
Using Webrev
Link to Webrev Comment