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

[Jim Graham]

Whoops, I didn't see this before...

Casts should be followed by a space as in:

(Foo) bar  // correct
(Foo)bar   // incorrect

			...jim

On 3/23/16 5:41 AM, Ajit Ghaisas wrote:
> Thanks Jim and Phil.
> Yes. It was a mistake to omit spaces in 'if' conditions. I have corrected the spacing issues for my changes now.
>
> I have corrected the indentation of the lines similar to those mentioned by Phil.
> I have also removed the redundant first condition check on line 890 in Raster.java mentioned as a review comment.
>
> Please review updated webrev :
> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.03/
>
> Regards,
> Ajit
>
> -----Original Message-----
> From: Phil Race
> Sent: Wednesday, March 23, 2016 4:40 AM
> To: Jim Graham
> Cc: Ajit Ghaisas; Sergey Bylokhov; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception
>
> 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.
>>>>>
>>>>
>>>>
>