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

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] Request for review: 7196571, 7196572, 7196573: javac warnings cleanup from Adopt Op
From:       martijnverburg () gmail ! com (Martijn Verburg)
Date:       2012-09-25 15:12:40
Message-ID: CAP7YuAR2ehWgnrPzc57jcCsZbesMvsxLFkpRx834gXYbabuKSA () mail ! gmail ! com
[Download RAW message or body]

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


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