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