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

[Philip Race]

 > 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.
>>