[OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception
Phil Race
philip.race at oracle.com
Tue Mar 29 18:56:17 UTC 2016
Looks good to me.
-phil.
On 03/28/2016 03:18 AM, Ajit Ghaisas wrote:
> Hi,
>
> Thanks Jim for thorough review.
>
> Here is the updated webrev :
> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.04/
>
> In this update :
> 1) I have corrected missing space after casting
> 2) I have modified code to adapt suggested indentation for blocks having conditionals & method declaration split on multiple lines.
>
> Please note that, I have done indentation change only if it is related to the code changes done as part of this fix.
> The files in this review already have indentation issues and fixing all of them will result in multiple changes masking the actual code changes that fixes the reported issue.
>
> Request you to review the updated webrev.
>
> Regards,
> Ajit
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, March 24, 2016 5:59 AM
> To: Phil Race
> Cc: 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception
>
> 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