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

Kurchi Hazra kurchi.subhra.hazra at oracle.com
Fri Feb 24 22:24:20 UTC 2012

Hi,

Please ignore the previous webrev and see this instead: http://cr.openjdk.java.net/~khazra/7146763/webrev.03/

This has Stuart's suggestion integrated correctly. In addition, I realized that make/sun/rmi/rmic/Makefile is not yet ready to have the JAVAC_WARNINGS_FATAL flag turned on, since it implicitly also builds files from sun/tools with more then 400 warnings in them. The change in this file has now been removed.

On 2/24/2012 11:01 AM, Kurchi Hazra wrote: > Hi Stuart, >> Thanks for the detailed explanation. Here is an updated webrev: > http://cr.openjdk.java.net/~khazra/7146763/webrev.02/ >>> - Kurchi >> 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

More information about the core-libs-dev mailing list