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

[Joe Darcy]

Agreed; looks good!

Thanks,

-Joe

On 8/11/2011 2:41 PM, Rémi Forax wrote:
> On 08/11/2011 11:25 PM, Alexandre Boulgakov wrote:
>> 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
>
> Looks good :)
>
> cheers,
> Rémi
>
>>
>> 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
>>>
>>>
>