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

[Rémi Forax]

On 02/24/2012 09: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

Hi Stuart, hi Kurchi,
sorry to not have answer before,
and yes, using asSubClass is better that what I've proposed.

cheers,
Rémi