[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR: 8255800: Raster creation methods need some specification clean up
From:       Phil Race <prr () openjdk ! java ! net>
Date:       2021-03-31 22:06:18
Message-ID: QUYqiu_kQLj5LPekov7bfqAg8AY5eDxWkwm130n84yo=.50e0b18f-df36-4729-8934-49d162dd37e2 () github ! com
[Download RAW message or body]

On Wed, 31 Mar 2021 21:21:24 GMT, Sergey Bylokhov <serb@openjdk.org> wrote:

> > https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line spec \
> > clean up but it didn't take a lot of looking to realize there were many more \
> > inconsistencies between spec and implementation. I've spent a lot of time on what \
> > is just small number of factory methods in Raster because there are so many \
> > possible exceptions and in some cases they rely on other API they call to \
> > generate exceptions and these may have not been documented or documented acc. to \
> > some long lost behavior. I've mostly tried to ONLY change spec. But I couldn't \
> > help myself when some checks were missed that ended up with bizarre and dubious \
> > behavior - throwing NegativeArrayIndexException which just about always has to be \
> > an internal bug ! 
> > The supplied test passes on JDK 16 as well as this code, because the (relatively) \
> > small number of cases where JDK 16 threw NegativeArrayIndexException are caught \
> > and allowed only for releases < 17 So where you see those in the test it \
> > corresponds to the behavioral changes from NegativeArrayIndexException to \
> > IllegalArgumentException. JCK conformance tests still pass so they must not test \
> > those conditions.
> 
> src/java.desktop/share/classes/java/awt/image/Raster.java line 263:
> 
> > 261:      *         both > 0
> > 262:      * @throws IllegalArgumentException if the product of {@code w}
> > 263:      *         and {@code h} is greater than {@code Integer.MAX_VALUE}
> 
> Do we really want to specify this? I hit this bug during HiDPI work and it seems \
> this is an implementation bug that we do not support big size(w*h).

The implementation issues that constrain this are substantial.
I find it unlikely we'd find a reason to support it.
I mean a bigger issue is we can'r even support an image that's Integer.MAX_VALUE wide \
and 1 pixel high.

Removing this from the spec. at that time would be trivial compared to the work in \
supporting it. And likely it would be some other method that would support that \
anyway for the aforementioned int reason. And we also (and always have) documented an \
exception if adding the location.x + w would overflow an int.  So in summary, yes, \
let's document it.

> src/java.desktop/share/classes/java/awt/image/Raster.java line 337:
> 
> > 335:      *         {@code DataBuffer.TYPE_BYTE},
> > 336:      *         {@code DataBuffer.TYPE_USHORT}
> > 337:      *         or {@code DataBuffer.TYPE_INT}
> 
> What about other types like TYPE_SHORT or we assume it is unsupported?

The existing code throws IAE if it is not one of these exact types. See around line \
460 in this updated file. And that code is in a public factory method called by this \
factory method. And that callee has exactly this documentation. So I am just copying \
it over so that it is consistently documented.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3223


[prev in list] [next in list] [prev in thread] [next in thread]