8028027: serialver should emit declaration with the 'private' modifier

[Stuart Marks]

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.marks at 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;
>>