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

[Joe Darcy]

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