[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [9] Review Request: 7188942 Remove support of pbuffers in OGL Java2d pipeline
From:       Jim Graham <james.graham () oracle ! com>
Date:       2015-06-29 23:33:28
Message-ID: 5591D5C8.1000506 () oracle ! com
[Download RAW message or body]

On 6/29/2015 10:35 AM, Sergey Bylokhov wrote:
> Hi, Jim.
> Thanks for review!
>
> Thew new version of the fix:
> http://cr.openjdk.java.net/~serb/7188942/webrev.02
>
> Comments about pbuffer were changed.
>> CGLGC.java:
>>
>> In createCompatVM() I dislike separated tests for "here is early
>> rejection of the list of things that I can handle" followed by a list
>> of tests for things it can handle.  For one thing we have to test the
>> type more than once. But mainly it just seems like the two tests can
>> get out of synch over time. It just seems cleaner to only iterate a
>> set of allowed types once. Wouldn't it be simpler and more
>> straightforward to do something like:
> The different ifs were merged. I think the code became more readable
> after that.

It's shorter, and it's fine, but personally I prefer to spell things 
out.  In particular, it gets confusing that your expression has two 
clauses that both test type ==/!= FB which causes me to make finger 
pictures in the air to pull it apart.  The cascading tests I wrote are 
more straightforward to follow and you can see right away that BITMASK 
is rejected regardless of type and that only FB and TEXTURE are allowed 
and that FB has one additional test.  But, that's just a question of 
style - at least the new version consolidates the tests into a single 
expression...

			...jim
[prev in list] [next in list] [prev in thread] [next in thread]