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
