On 9/11/12 1:41 PM, Martijn Verburg wrote:
* 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).
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.
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
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.
s'marks
