Hi Yuri,

I don't see any consistency issue, since serialver and javap are intended for different purposes.

I checked with a couple reviewers from earlier in this thread and they're ok with the change as is, without the additional reflective code to pick up modifiers from an existing declaration, so I've pushed the original change.

Thanks for the comments though.

s'marks

On 11/8/13 9:52 PM, Yuri Gaevsky wrote:
Stuart,

Sorry, but such inconsistency between serialver/javap is a bug (IMHO, of 
course).

If there happens to be a declaration in the class that, probably mistakenly, 
goes against this advice, serialver shouldn't emit a line that perpetuates this 
mistake.

I would argue that for any real-life API it's almost impossible to fix such 
explicitly-defined "bad"  (i.e. public/protected) SVUID in any compatible way,  
so emitting 'private'  will not help there, unfortunately.

-Yuri

-----Original Message-----
From: Stuart Marks [mailto:stuart.ma...@oracle.com]
Sent: Friday, November 8, 2013 10:48 PM
To: Yuri Gaevsky
Cc: Alan Bateman; core-libs-dev
Subject: Re: 8028027: serialver should emit declaration with the 'private' 
modifier

On 11/8/13 7:20 AM, Yuri Gaevsky wrote:
Well, it would be more consistent to check for existence of protected or public 
serialVersionUID with Reflection API and change the serialver output 
accordingly.

Please see suggested fix and its output below.

This change isn't consistent with the intent of the 'serialver' utility. Its intent is to produce a 
declaration that's "suitable for copying into an evolving class." [1] (Clearly, this 
means the source code of a class.) The spec [2] "strongly advises" that serialVersionUID 
be private. As such, serialver should follow the strong advice given in the spec.

If there happens to be a declaration in the class that, probably mistakenly, 
goes against this advice, serialver shouldn't emit a line that perpetuates this 
mistake.

One can use "javap -v" to determine the presence of serialVersionUID field, 
including its modifiers, if that's what's desired.

s'marks

[1] http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/serialver.html

[2] http://docs.oracle.com/javase/7/docs/api/java/io/Serializable.html



Thanks,
-Yuri

$ serialver java.security.PublicKey
java.security.PublicKey:    public static final long serialVersionUID = 
7187392471159151072L;

$ serialver java.lang.Exception
java.lang.Exception:     static final long serialVersionUID = 
-3387516993124229948L;

$ serialver java.lang.AssertionError
java.lang.AssertionError:    private static final long serialVersionUID = 
-5013299493970297370L;

$ serialver javax.xml.ws.soap.SOAPFaultException
javax.xml.ws.soap.SOAPFaultException:    private static final long 
serialVersionUID = -104968645459360720L;


$ hg diff
diff --git a/src/share/classes/sun/tools/serialver/SerialVer.java
b/src/share/classes/sun/tools/serialver/SerialVer.java
--- a/src/share/classes/sun/tools/serialver/SerialVer.java
+++ b/src/share/classes/sun/tools/serialver/SerialVer.java
@@ -38,6 +38,7 @@
   import java.net.MalformedURLException;
   import java.util.StringTokenizer;
   import sun.net.www.ParseUtil;
+import java.lang.reflect.Modifier;

   public class SerialVer extends Applet {
       GridBagLayout gb;
@@ -211,7 +212,17 @@
           Class<?> cl = Class.forName(classname, false, loader);
           ObjectStreamClass desc = ObjectStreamClass.lookup(cl);
           if (desc != null) {
-            return "    static final long serialVersionUID = " +
+           String ams = "";
+           try {
+               final int mod =
+                       cl.getDeclaredField("serialVersionUID").getModifiers();
+               ams = Modifier.isPublic(mod) ? "public"
+                   : Modifier.isProtected(mod) ? "protected"
+                   : Modifier.isPrivate(mod) ? "private" : "";
+           } catch (NoSuchFieldException nsfe) {
+               ams = "private";
+           }
+            return "    " + ams + " static final long serialVersionUID = " +
                   desc.getSerialVersionUID() + "L;";
           } else {
               return null;

Reply via email to