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

[Kurchi Hazra]

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