Code review request: 7077389 Reflection classes do not build with javac -Xlint:all -Werror

[Rémi Forax]

On 08/10/2011 10:43 PM, Alexandre Boulgakov wrote:
> Hello Remi,
>

Hello,

> Thanks for the feedback!
>
> On 8/10/2011 1:22 PM, Rémi Forax wrote:
>> On 08/10/2011 09:39 PM, Joe Darcy wrote:
>>> On 8/10/2011 11:36 AM, Alexandre Boulgakov wrote:
>>>> Hello Joe,
>>>>
>>>> Could you please review these small changes to remove javac build 
>>>> warnings from the reflection classes?
>>>>
>>>> Bug detail: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7077389
>>>> webrev: http://cr.openjdk.java.net/~mduigou/7077389/0/webrev/ 
>>>> <http://cr.openjdk.java.net/%7Emduigou/7077389/0/webrev/>
>>>>
>>>> I haven't updated the copyrights to avoid polluting the webrev.
>>>> I did not add JAVAC_WARNINGS_FATAL=true to the makefile because it 
>>>> is responsible for other packages which still have build warnings.
>>>>
>>>> Thanks,
>>>> Sasha
>>>
>>> Hi Sasha.
>>>
>>> I approve the code changes with the following exceptions:
>>>
>>>     src/share/classes/sun/reflect/UnsafeFieldAccessorImpl.java
>>>
>>> Offhand, I'm not sure of the semantics of this change.
>>>
>>> While adding serialVersionUIDs in sun/reflect/annotation/* is a good 
>>> change, I believe this interacts with another bug I'm working on so 
>>> please hold off on those changes for now.
>>>
>>> Thanks,
>>>
>>> -Joe
>>
>> In AccessorGenerator:
>>
>>   protected static final Class<?>[] primitiveTypes = new Class[] {
>>
>> should be
>>
>>   protected static final Class<?>[] primitiveTypes = new Class<?>[] {
> Generic array creation is not currently supported (here and for your 
> other suggestions). 

If the generics is an unbound wildcard, there is no problem.
To reuse the JLS words, a type parametrized by an unbound wildcard is 
reified.

> Additionally, if I'm not mistaken, wildcards are limited to variable 
> declarations, since they wouldn't make sense on a constructor or 
> method call.

wildcards can be used only inside a parameterized type,  whenever you 
use that type to define
a local variable or the type used by new.
Also you can't use a wildcard as an argument type, i.e. you can't 
substitute a T to a ?.

So here, there is no problem.

>>
>> in AnnotationParser:
>>
>>   type.getClassLoader(), new Class[] { type },
>>
>> should be
>>
>>   type.getClassLoader(), new Class<?>[] { type },
>>
>>
>> in TypeVariableImpl:
>>
>>   TypeVariable<? extends GenericDeclaration>  that =
>>                     (TypeVariable<? extends GenericDeclaration>) o;
>>
>> should be
>>      TypeVariable<?>  that =  (TypeVariable<?>) o;
> I believe that it is more clear why we can assign the result of 
> that.getGenericDeclaration() to a GenericDeclaration if we include the 
> " extends GenericDeclaration" constraint on the wildcard.

but here you fall into a corner case of the spec,
a cast to TypeVariable<?> is safe (again it's a reified type) but
a cast to TypeVariable<? extends GenericDeclaration> is not safe
even if TypeVariable is declared like this:
   class TypeVariable<D extends GenericDeclaration> { ... }

>
> -Sasha

Rémi

>>
>>
>> In GenericDeclRepository:
>>
>>   @SuppressWarnings("rawtypes")
>>             TypeVariable<?>[] tps = new TypeVariable[ftps.length];
>>
>> should be
>>             TypeVariable<?>[] tps = new TypeVariable<?>[ftps.length];
>>
>>
>> It seems that the compiler is not able to find a raw type
>> if it's the component type of an array.
>> (CC to compiler-dev)
>> cheers,
>> Rémi
>>