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

[Jim Graham]

Actually, it was sufficient to make that apparent, but I again fell 
under the spell of "all these methods are the same, so the same logic 
should apply in each of them".

Apologies again for the non-sequitur.  If it's a DBB, then the type must 
be TYPE_BYTE...

			...jim

On 3/29/2016 8:59 PM, Philip Race wrote:
>  > One comes from the SampleModel, the other comes from the DataBuffer.
>
> In the other methods that is true but not in this one.
> This method creates the SampleModel using the type of the DataBuffer.
> The webrev diff is insufficient to make this apparent.
>
> -phil.
>
> On 3/29/16, 7:17 PM, Jim Graham wrote:
>> One comes from the SampleModel, the other comes from the DataBuffer.
>> The reason to check them both is to make sure they agree.  There is no
>> "it must be DataBuffer.TYPE_BYTE" here, there is only "it *should* be
>> DataBuffer.TYPE_BYTE" and that warrants a test and a fallback to the
>> generic SunWritableRaster if they ever send in the wrong type.
>>
>> Otherwise why do we bother to switch on the dataType in the other
>> sections of the code before we check instanceof?  For the same reason
>> as we need both checks here.  If we expanded out this final else
>> clause in parallel to match the way that the other blocks are written,
>> then we would have:
>>
>>     } else if (sm instanceof MPPSM) {
>>             switch (dataType) {
>>                 case DataBuffer.TYPE_BYTE:
>>                     if (dataBuffer instanceof DataBufferByte) {
>>                         if (sm.getSampleSize(0) < 8) {
>>                             return new BPP(...);
>>                         }
>>                     }
>>                     break;
>>             }
>>         }
>>
>> The tests in the prior version were a perfect compaction of all of the
>> above constructs into a single if statement.  The new one omits the
>> switch test that all of the other blocks have...
>>
>>             ...jim
>>
>> On 3/29/16 3:31 PM, Phil Race wrote:
>>> On 03/29/2016 02:37 PM, Jim Graham wrote:
>>>> Raster.java, line 894: Why was the test for dataType removed from this
>>>> if statement?
>>>
>>> In a  previous version  ..
>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.02/src/java.desktop/share/classes/java/awt/image/Raster.java.sdiff.html
>>>
>>>
>>>
>>> Ajit had added a condition :
>>>
>>> 890         if (dataType == DataBuffer.TYPE_BYTE &&
>>> 891             dataBuffer instanceof DataBufferByte &&
>>>
>>>
>>> But we don't need both of these.
>>> DataBufferByte should always be DataBuffer.TYPE_BYTE and we do the cast
>>> so it had better be DataBufferByte.
>>>
>>> -phil.
>>>