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

[Kurchi Hazra]

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.

- Kurchi



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