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

[Stuart Marks]

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();