[OpenJDK 2D-Dev] Request for review: 7196571, 7196572, 7196573: javac warnings cleanup from Adopt OpenJDK bugathon

[Martijn Verburg]

Hi Phil

Thanks for taking a look at these so quickly.

> I reviewed all the changes and they look fine to me with just formatting
> issues and one code change question or suggestion
>
> -* an identation issue at line 596 here
>
> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.imageio/src/share/classes/com/sun/imageio/plugins/common/ImageUtil.java.sdiff.html
>
> The removal of the cast means the other lines aren't aligned any more
>
> * I also wonder why lines 837 & 838 can't be collapsed into one line ?
> 0x00000001 << (31 - bOffset % 32));

Good point, I've fixed both of those.

> * By contrast lines 1035 and 1115 here are now much, much > 80 chars
> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.imageio/src/share/classes/javax/imageio/ImageIO.java.sdiff.html
>
> Also for both of these cases, you added .asSubClass(..).
> Now when I look at the new code there, it looks a lot busier, all to avoid
> one
> simple (ImageWriterSpi) cast. Is it really worth it ?
> And if the change is made, maybe you should check for ClassCastException.
> I guess we already vulnerable to that, although its rather unlikely to occur
> ...

I'm more than happy to follow your lead on this one. I'll revert those
two, we can
always visit it again later with fresh eyes (I'm not that convinced that
using asSubClass(..) is the way to go here either).

I've sent in an updated patch to Artem so he can update the webrev

> I double checked for most of the serial verson uids in the imaging &
> printing classes that they were generated from running serialver on
> the classes so as to maintain the existent values, so that looks correct.
>
> Don't forget you need a second reviewer for all the changes.

OK, thanks - if another reviewer is following this thread then any
assistance would be
most welcome :-)

Cheers,
Martijn

> On 9/11/2012 11:40 AM, Martijn Verburg wrote:
>>
>> Hi all,
>>
>> Artem Ananiev very kindly raised bugs and a webrev for the patches
>> sent in from a Bugathon we ran back in April (patches have been tested
>> against latest source tree).
>>
>> The bug numbers are:
>>
>> 7196571: [Bugathon] Reduce the number of javac warnings in ImageIO
>> 7196572: [Bugathon] Reduce the number of javac warnings in color
>> management
>> 7196573: [Bugathon] Reduce the number of javac warnings in imaging
>>
>> The corresponding webrevs are at:
>>
>> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.imageio/
>> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.j2d-color/
>> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.j2d-image/
>> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.j2d-print/
>>
>> Thanks to Artem, Stuart and Phil for helping me navigate through the
>> AWT/2D waters :-)
>>
>> Cheers,
>> Martijn
>
>