Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Fri Feb 24 19:01:18 UTC 2012

Hi Stuart,

Thanks for the detailed explanation. Here is an updated webrev:

On 2/24/2012 12:54 AM, Stuart Marks wrote: > On 2/22/12 1:25 PM, Kurchi Hazra wrote: >> On 2/22/2012 10:01 AM, Rémi Forax wrote: >>> Hi Kurchi, hi all, >>>>>> in ReliableLog, you can get ride of the @SupressWarnings, >>> getLogClassConstructor should return a Constructor<?> and not a >>> _Constructor>> _extends LogFile>,_ >>> _the field logClassConstructor should be typed Constructor and_ >>> in openLogFile, the log should be constructed like this >>>>>> log = (logClassConstructor == null ? >>> new LogFile(logName, "rw") : >>>>>> (LogFile)logClassConstructor.newInstance(logName, "rw")); >>>>>> The idea is that a cast on a LogFile is typesafe but not a cast on a >>> Constructor<? extends LogFile>. >>>> If I change the return type to Constructor<?>, I get the following >> error: >> ../../../../src/share/classes/sun/rmi/log/ReliableLog.java:122: error: >> incompatible types >> logClassConstructor = getLogClassConstructor(); >> ^ >> required: Constructor<? extends LogFile> >> found: Constructor<CAP#1> >> where CAP#1 is a fresh type-variable: >> CAP#1 extends Object from capture of ? >> And the following warning: >>>> ../../../../src/share/classes/sun/rmi/log/ReliableLog.java:350: warning: >> [unchecked] unchecked cast >> cl.getConstructor(String.class, >> String.class); >> ^ >> required: Constructor<? extends LogFile> >> found: Constructor<CAP#1> >> where CAP#1 is a fresh type-variable: >> CAP#1 extends Object from capture of ? >>>>>> Thanks, >> Kurchi >> Hi Kurchi, >> To implement Rémi's suggestion fully, you would also have to change > the type of logClassConstructor to Contructor<?> near line 122, remove > the cast of cl.getConstructor() near line 350, and then add the cast > to LogFile at the call to newInstance() near line 546. >> This works to get rid of the warnings and errors, but the declaration > of Constructor<?> is somewhat imprecise. The code checks to make sure > that the loaded class is a subclass of LogFile (that's what the > isAssignableFrom check is doing). Thus the type of the loaded class > really should be Class<? extends LogFile>, and correspondingly the > logClassConstructor should be Constructor<? extends LogFile>. That's > how logClassConstructor is declared now and it would be nice to keep > it that way. >> It turns out that Class.asSubclass() does this conversion without > generating an unchecked warning. This internally does an > isAssignableFrom() check and casts to the right wildcarded type, so > this can simplify the code in getLogClassConstructor() somewhat as > well. (Incidentally, asSubClass() has @SuppressWarnings on its > implementation.) I've appended some diffs below (to be applied on top > of your most recent webrev) to show how this can be done. >> The behavior is slightly different, as it throws ClassCastException > (which is caught by the catch clause below, emitting a log message) > instead of silently returning null. This is probably an improvement, > since if the user specifies the wrong class in the property name, the > exception stack trace should indicate what happened. >> s'marks >>>>> diff -r 72d32fd57d89 src/share/classes/sun/rmi/log/ReliableLog.java > --- a/src/share/classes/sun/rmi/log/ReliableLog.java Fri Feb 24 > 00:01:53 2012 -0800 > +++ b/src/share/classes/sun/rmi/log/ReliableLog.java Fri Feb 24 > 00:39:02 2012 -0800 > @@ -330,9 +330,7 @@ > * property a) can be loaded, b) is a subclass of LogFile, and c) > has a > * public two-arg constructor (String, String); otherwise returns > null. > **/ > - @SuppressWarnings("unchecked") > - private static Constructor<? extends LogFile> > - getLogClassConstructor() { > + private static Constructor<? extends LogFile> > getLogClassConstructor() { >> String logClassName = AccessController.doPrivileged( > new GetPropertyAction("sun.rmi.log.class")); > @@ -345,11 +343,9 @@ > return > ClassLoader.getSystemClassLoader(); > } > }); > - Class<?> cl = loader.loadClass(logClassName); > - if (LogFile.class.isAssignableFrom(cl)) { > - return (Constructor<? extends LogFile>) > - cl.getConstructor(String.class, > String.class); > - } > + Class<? extends LogFile> cl = > + > loader.loadClass(logClassName).asSubclass(LogFile.class); > + return cl.getConstructor(String.class, String.class); > } catch (Exception e) { > System.err.println("Exception occurred:"); > e.printStackTrace(); >>

-Kurchi