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

[Martijn Verburg]

Hi all,

Apologies for the long response, as with many of you JavaOne prep
seems to get in the way of doing interesting coding work!

On 12 September 2012 20:03, Stuart Marks <stuart.marks at oracle.com> wrote:
> On 9/11/12 1:41 PM, Martijn Verburg wrote:
>>>
>>> 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).
>
> Using Class.forName(...).asSubClass(...) is a reasonable type-safe way of
> getting a Class<T> given a string name. However, it does change the location
> of any exception that might get thrown, and there might be a subtle
> interaction with the IIORegistry, so it might indeed be best to leave these
> untouched for the moment.

OK, left untouched in that case.

> I looked through the webrevs and I noticed a couple minor things...
>
> *
> http://cr.openjdk.java.net/~art/Bugathon-2012/webrev.j2d-print/src/share/classes/sun/print/RasterPrinterJob.java.sdiff.html
>
>  - lines 508-509 can be joined
>  - lines 1311-1312 should probably be broken after the = operator,
>    for consistency with other cases here
>  - lines 1320-1321 can be joined
>  - line 1910 can remove redundant parentheses

I've fixed these and sent Artem an updated patch for his webrev

>>> 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 :-)
>
> I'm not sure I count as a second reviewer. I did look quickly through all
> the changes, though, and the only issues I noticed were the ones I mentioned
> above.

Hopefully that's all that's required then :-)

Cheers,
Martijn