[OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception
Jim Graham
james.graham at oracle.com
Thu Mar 24 00:29:07 UTC 2016
I should point out that this form I demonstrate below is only used when
there are continuation lines on the prefix to the code block
(conditionals, method declarations, loop controls, etc.), so:
if (some simple condition) {
...
}
and:
if (some complex condition &&
some additional condition)
{
...
}
but, not:
if (some single line condition)
{
// blech
...
}
The reason for this is that the standard indentation would recommend:
if (some complex condition &&
some additional condition) {
// code block
}
or
void foomethod(int sx, int sy,
int dx, int dy) {
// code block
}
which may be more compact, but the lack of a breaking line means you
have to vary the indentation of the declarations/conditionals and as a
result they don't line up nicely which makes them harder to scan if they
are all related (and frequently in graphics code you end up with a
series of very similar arguments or conditionals that are more readable
if they line up nicely), and the only indication of when the multiple
lines of continued declaration/conditionals end and the body of the
method begins is the number of spaces - noting that frequently the
indentation on lines in practice is just wrong so this form makes it
hard to distinguish between "that line is a continuation line" and
"someone indented that line wrong"...
...jim
On 3/23/16 5:14 PM, Jim Graham wrote:
> 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.
>>>>>>
>>>>>
>>>>>
>>
More information about the 2d-dev
mailing list