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

[Alexandre Boulgakov]

Hello, Joe and Rémi:

Please review this updated webrev that incorporates your suggestions.
http://cr.openjdk.java.net/~mduigou/7077389/1/webrev/ 
<http://cr.openjdk.java.net/%7Emduigou/7077389/1/webrev/>

Specifically, I:

  * removed the 4 serialVersionUIDs I added;
  * used "new TypeName<?>[...]" in place of "new TypeName[...]" and
    removed associated SuppressWarnings annotations; and
  * changed TypeVariable<? extends GenericDeclaration> to
    TypeVariable<?> since the conversion would otherwise be unchecked.


Thanks,
Sasha

On 8/11/2011 9:19 AM, Joe Darcy wrote:
> Alexandre Boulgakov wrote:
>> Hello Joe,
>>
>> Thanks for reviewing and responding so quickly!
>>
>> On 8/10/2011 12: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.
>> Do you mean changing the type of fieldOffset or the change in the 
>> constructor? I changed fieldOffset from an int to a long per the 
>> specification change mentioned in Unsafe.staticFieldOffset(Field) and 
>> Unsafe.objectFieldOffset(Field). The change in the constructor is 
>> related and stems from the fact that Unsafe.fieldOffset(Field) is now 
>> deprecated. Since sun.reflect.UnsafeFieldAccessorImpl is 
>> package-private, this change should not have any external effects.
>
> Okay; I wasn't aware of the reasoning for this change and I noted the 
> int -> long field change; I think that change is fine then.
>
>
>
>>
>>>
>>> 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.
>> Since I only have one week left in my internship, I would like to get 
>> the changes in as quickly as possible. Should I remove the 
>> serialVersionUIDs but keep the remaining changes?
>
> Yes.
>
> Thanks,
>
> -Joe
>
>