Code review request: 7146763: Warnings cleanup in the sun.rmi and related packages (original) (raw)

Rémi Forax forax at univ-mlv.fr
Fri Feb 24 09:00:00 UTC 2012

On 02/24/2012 09: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

Hi Stuart, hi Kurchi, sorry to not have answer before, and yes, using asSubClass is better that what I've proposed.

cheers, Rémi

More information about the core-libs-dev mailing list