[OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception

[Jim Graham]

For the record, in many places in the 2D code we also adopt a slight 
extension of the indentation rules such that the below might be written as:

     public ByteBandedRaster(SampleModel sampleModel,
                             DataBufferByte dataBuffer,
                             Point origin)
     {
         // body lines of the method...
     }

with the open curly brace on the following line so that it more visually 
points out the beginning of the body block of the method and it's easy 
to find the start/end of the block by sighting down the left margin. 
The official policy is for the "{" to be at the end of the previous line 
after "Point origin) {" and as more new engineers work on the code and 
follow the official rules, the above form becomes less common.  (Sad 
face since I particularly like the above form...)

			...jim

On 3/22/16 4:10 PM, Phil Race wrote:
> Ajit,
>
> There is also some odd indentation in ByteBandedRaster.java which is not
> yours but
>
>   98     public ByteBandedRaster(SampleModel sampleModel,
>    99                                DataBufferByte dataBuffer,
>   100                                Point origin) {
>
>
> This appears to be the result of someone using tabs a long time ago.
>
> Since you are touching the method signature lines anyway may be
> better fixed so we have these aligned
>
>       public ByteBandedRaster(SampleModel sampleModel,
>                               DataBufferByte dataBuffer,
>                               Point origin) {
>
> [not sure if that will make it through mail as intended].
>
> Here in Raster.java, the first condition now seems to be redundant ..
> 890         if (dataType == DataBuffer.TYPE_BYTE &&
>   891             dataBuffer instanceof DataBufferByte &&
>
>
>
> -phil.
>
>
> On 03/22/2016 03:28 PM, Jim Graham wrote:
>> Hi Ajit,
>>
>> Most of your if statements are spaced wrong.  There should be a space
>> between "if" and the parentheses.  I'll review more later, but I
>> noticed that issue in the first couple of files I looked at...
>>
>>         ...jim
>>
>> On 3/15/16 7:05 AM, Ajit Ghaisas wrote:
>>> Hi,
>>>
>>>     Thanks Sergey and Jim for suggestions.
>>>
>>>      I have made the data specific Raster constructors type safe
>>> now.  Also, I have modified all Raster creations in Raster.java to
>>> support custom DataBuffer classes.
>>>
>>>      Please review the changes present in updated webrev :
>>>      http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.02/
>>>
>>> Regards,
>>> Ajit
>>>
>>> -----Original Message-----
>>> From: Jim Graham
>>> Sent: Friday, March 11, 2016 2:40 AM
>>> To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev
>>> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518:
>>> Creation of a WritableRaster with a custom DataBuffer causes
>>> erroneous Exception
>>>
>>> Yes, those constructors should be type-safe.  Perhaps that was done
>>> to save code in the caller, but that is a small price to pay to avoid
>>> errors in the future, and it makes sure there is a backup plan for
>>> different kinds of buffers...
>>>
>>>             ...jim
>>>
>>> On 3/10/16 4:48 AM, Sergey Bylokhov wrote:
>>>> Hi, Ajit.
>>>> What about other cases in Raster.java, where the DataBuffer is passed
>>>> as a parameter? Probably we can simplify the code and find all such
>>>> issues if we changes the ByteInterleavedRaster/etc. For example:
>>>>
>>>> ByteInterleavedRaster(SampleModel sampleModel,DataBuffer
>>>> dataBuffer,...) to
>>>>
>>>> ByteInterleavedRaster(SampleModel sampleModel,DataBufferByte
>>>> dataBuffer,...)
>>>>
>>>> Because we throw an exception in such cases anyway:
>>>>
>>>> if (!(dataBuffer instanceof DataBufferByte)) {
>>>>       throw new RasterFormatException("ByteInterleavedRasters must
>>>> have "
>>>> + "byte DataBuffers");
>>>> }
>>>>
>>>> And the compiler will help us, what everybody think about it?
>>>>
>>>> On 09.03.16 17:38, Ajit Ghaisas wrote:
>>>>> Hi,
>>>>>
>>>>>      Modified the test and added check for MultiPixelPackedSampleModel
>>>>> condition.
>>>>>
>>>>>      Please review updated webrev :
>>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.01/
>>>>>
>>>>> Regards,
>>>>> Ajit
>>>>>
>>>>> -----Original Message-----
>>>>> From: Sergey Bylokhov
>>>>> Sent: Wednesday, March 09, 2016 4:06 PM
>>>>> To: Ajit Ghaisas; awt-dev at openjdk.java.net; Semyon Sadetsky; Ambarish
>>>>> Rapte; 2d-dev
>>>>> Subject: Re: [9] Review-request for 6353518: Creation of a
>>>>> WritableRaster with a custom DataBuffer causes erroneous Exception
>>>>>
>>>>> Changes for 2d area.(cc)
>>>>>
>>>>>
>>>>> On 07.03.16 11:20, Ajit Ghaisas wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-6353518
>>>>>>
>>>>>> Issue : (Text from bug description)
>>>>>>
>>>>>> An attempt to create a WritableRaster via
>>>>>> Raster.createWritableRaster(SampleModel sm, DataBuffer db, Point
>>>>>> location) using a custom DataBuffer causes an erroneous
>>>>>> RasterFormatException.
>>>>>> Apparently the reason for this bug is that IntegerComponentRaster
>>>>>> insists on being passed an instance of DataBufferInt rather than
>>>>>> just a DataBuffer with a DataType of TYPE_INT.
>>>>>> This is quite annoying since DataBufferInt is declared final and
>>>>>> thus cannot be extended.
>>>>>>
>>>>>> Fix :
>>>>>>
>>>>>> The last line of Raster.createWritableRaster() method already has a
>>>>>> way to handle generic case if the input does not match any of the
>>>>>> cases in switch statements in that method.
>>>>>>
>>>>>> The fact that  " *InterleavedRaster() constructors throw exception
>>>>>> if DataBuffer passed to them does not match the expected type" was
>>>>>> ignored earlier.
>>>>>>
>>>>>> This fix adds a check of "DataBuffer type" before creating
>>>>>> respective
>>>>>> *InterleavedRaster() objects.
>>>>>>
>>>>>> It constructs the SunWritableRaster in case DataBuffer type does not
>>>>>> match any data specific DataBuffer classes (DataBufferByte,
>>>>>> DataBufferUShort, DataBufferInt)
>>>>>>
>>>>>> Request to review webrev containing this fix :
>>>>>>
>>>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.00/
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Ajit
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>>>
>>>>
>>>>
>