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

[Rémi Forax]

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
>>
>>