Hi Ajit,
On 3/30/2016 5:12 AM, Ajit Ghaisas wrote:
Raster.java, line 986: Something to file as a potential bug for future work
since the fix would have to be verified that it doesn't disrupt the other parts
of this method, but... The set of if statements in this method never checked
for a BandedSampleModel to try to create a BandedRaster as in
createBandedRaster. On the other hand, no developer has complained so far
(that I know of, but maybe we have an old bug filed on it that I'm not aware
of).
[Ajit] : Without my fix, it was easier to get RasterFormatException as
explained in Bug Description. This fix enhances the code by making type
specific Raster class constructors type-safe.
Now, the bigger question is: Whether methods in Raster.java support all
possible combinations of Raster creations?
What I can say is - this fix does not break anything than what is already
present in Raster.java.
If there is specific user complaint or any existing bug - we can look into it
separately.
I agree that this creates no new failure modes, which is why I suggested
it as something to file for future work. (an enhancement)
The following 2 comments are identical for all of the raster types {
ByteBandedRaster.java, line 76: This code will throw a ClassCastException
now instead of the RasterFormatException that used to be thrown.
Do we care? It would be easy enough to add a test to just this
method (and the similar versions in other files).
[Ajit] : Since we have decided to make the type specific Raster class constructors type
safe, yes, it is possible to get ClassCastException. But, as the usage of these classes
is guarded now with "instanceof" checks, it would be rare.
In other cases, where the caller supplies the data buffer, we have
protected them all with instance checks, but in this case the method is
relying on the sample model to create the correct type of data buffer.
If this method were called from a random place then we'd have no
guarantees, but as far as I can see, it is only called from within
ByteBandedRaster itself and it is called on the same sample model that
it was originally created with.
The typical case will probably not be an issue, but it's not immediately
clear if there is any way that someone can end up with a custom sample
model in a ByteBandedRaster that produces some other kind of data
buffer? Possibly this would be more likely if there were any other
callers to that method, but a search of java.desktop didn't turn up any.
It would be difficult at best to engineer a test case that would
discover the difference here and that test case would have previously
thrown an exception anyway so I suppose I'm not going to be concerned if
the type of the exception has changed here...
ByteBandedRaster.java, line 668: Extra credit: This cast could be avoided
if we make SunWritableRaster employ generics for a strongly typed
DataBuffer.
[Ajit] : the casted member ' dataBuffer' on this line is a member of Raster
class.
The class hierarchy of ByteBandedRaster is ( added on single line for
convenience)
-------- ByteBandedRaster extends SunWritableRaster extends WritableRaster
extends Raster.
I am not quite clear on how we can avoid cast on this line. Can you please
elaborate?
}
The object being cast here is the contents of the dataBuffer field of
the raster, which is known to be a DataBufferByte for the case of
ByteBandedRaster because all of its constructors enforce that type,
especially so now that the constructors are all type-specific. If all
of the super-classes were generified for the kind of DataBuffer that
they held then we wouldn't need a cast here.
But, that would be a public API change since the field is stored in the
public java.awt.image.Raster class, so the generification would have to
extend all the way up to that class.
Something to consider for the future, but definitely outside the scope
of this fix. And if we do that, then we should also consider other use
of generics within all of the java.awt.image classes and that would be a
much larger effort.
Here is the updated webrev:
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.05/
If you only changed those things you mentioned above then it looks good
to go. If there is something else that was changed, let me know and I'll
do a more thorough review...
...jim
