8177738: Runtime.Version must be a value-based class
authorprappo
Fri, 21 Apr 2017 19:13:47 +0100
changeset 44785 62a18e20f5c1
parent 44784 f2e3396a6a6b
child 44786 99d2f0019a0a
8177738: Runtime.Version must be a value-based class 8148822: (spec) Regex in Runtime.Version and JEP 223 should match 8160954: (spec) Runtime.Version regex and $PRE/$OPT issues 8148877: (spec) Specify when an empty '+' is required in a version string Reviewed-by: psandoz, rriggs Contributed-by: Iris Clark <iris.clark@oracle.com>, Pavel Rappo <pavel.rappo@oracle.com>
jdk/src/java.base/share/classes/java/lang/Runtime.java
jdk/src/java.base/share/classes/java/lang/VersionProps.java.template
jdk/test/java/lang/Runtime/Version/Basic.java
--- a/jdk/src/java.base/share/classes/java/lang/Runtime.java	Thu Apr 20 22:46:22 2017 -0700
+++ b/jdk/src/java.base/share/classes/java/lang/Runtime.java	Fri Apr 21 19:13:47 2017 +0100
@@ -930,10 +930,9 @@
     }
 
     /**
-     * Returns the version of the Java Runtime Environment as a {@link
-     * Runtime.Version}.
+     * Returns the version of the Java Runtime Environment as a {@link Version}.
      *
-     * @return  the {@link Runtime.Version} of the Java Runtime Environment
+     * @return  the {@link Version} of the Java Runtime Environment
      *
      * @since  9
      */
@@ -948,7 +947,7 @@
 
     /**
      * A representation of a version string for an implementation of the
-     * Java&nbsp;SE Platform.  A version string contains a version number
+     * Java&nbsp;SE Platform.  A version string consists of a version number
      * optionally followed by pre-release and build information.
      *
      * <h2><a name="verNum">Version numbers</a></h2>
@@ -960,7 +959,7 @@
      * </p>
      *
      * <blockquote><pre>
-     *     ^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$
+     *     [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
      * </pre></blockquote>
      *
      * <p> The sequence may be of arbitrary length but the first three
@@ -1026,10 +1025,13 @@
      *
      * <p> A <em>version string</em>, {@code $VSTR}, consists of a version
      * number {@code $VNUM}, as described above, optionally followed by
-     * pre-release and build information, in the format </p>
+     * pre-release and build information, in one of the following formats:
+     * </p>
      *
      * <blockquote><pre>
-     *     $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?
+     *     $VNUM(-$PRE)?\+$BUILD(-$OPT)?
+     *     $VNUM-$PRE(-$OPT)?
+     *     $VNUM(+-$OPT)?
      * </pre></blockquote>
      *
      * <p> where: </p>
@@ -1039,17 +1041,17 @@
      * <li><p> <a name="pre">{@code $PRE}</a>, matching {@code ([a-zA-Z0-9]+)}
      * --- A pre-release identifier.  Typically {@code ea}, for a
      * potentially unstable early-access release under active development,
-     * or {@code internal}, for an internal developer build.
+     * or {@code internal}, for an internal developer build. </p></li>
      *
      * <li><p> <a name="build">{@code $BUILD}</a>, matching {@code
      * (0|[1-9][0-9]*)} --- The build number, incremented for each promoted
      * build.  {@code $BUILD} is reset to {@code 1} when any portion of {@code
-     * $VNUM} is incremented. </p>
+     * $VNUM} is incremented. </p></li>
      *
      * <li><p> <a name="opt">{@code $OPT}</a>, matching {@code
-     * ([-a-zA-Z0-9\.]+)} --- Additional build information, if desired.  In
+     * ([-a-zA-Z0-9.]+)} --- Additional build information, if desired.  In
      * the case of an {@code internal} build this will often contain the date
-     * and time of the build. </p>
+     * and time of the build. </p></li>
      *
      * </ul>
      *
@@ -1067,15 +1069,21 @@
      *
      * <p> A <em>short version string</em>, {@code $SVSTR}, often useful in
      * less formal contexts, is a version number optionally followed by a
-     * pre-release identifier:
+     * pre-release identifier:</p>
      *
      * <blockquote><pre>
      *     $VNUM(-$PRE)?
      * </pre></blockquote>
      *
+     * <p>This is a <a href="./doc-files/ValueBased.html">value-based</a>
+     * class; use of identity-sensitive operations (including reference equality
+     * ({@code ==}), identity hash code, or synchronization) on instances of
+     * {@code Version} may have unpredictable results and should be avoided.
+     * </p>
+     *
      * @since  9
      */
-    public static class Version
+    public static final class Version
         implements Comparable<Version>
     {
         private final List<Integer>     version;
@@ -1083,9 +1091,18 @@
         private final Optional<Integer> build;
         private final Optional<String>  optional;
 
-        Version(List<Integer> version, Optional<String> pre,
-                Optional<Integer> build, Optional<String> optional) {
-            this.version = Collections.unmodifiableList(version);
+        /*
+         * List of version number components passed to this constructor MUST
+         * be at least unmodifiable (ideally immutable). In the case on an
+         * unmodifiable list, the caller MUST hand the list over to this
+         * constructor and never change the underlying list.
+         */
+        private Version(List<Integer> unmodifiableListOfVersions,
+                        Optional<String> pre,
+                        Optional<Integer> build,
+                        Optional<String> optional)
+        {
+            this.version = unmodifiableListOfVersions;
             this.pre = pre;
             this.build = build;
             this.optional = optional;
@@ -1129,9 +1146,11 @@
                                                    + s + "'");
 
             // $VNUM is a dot-separated list of integers of arbitrary length
-            List<Integer> version = new ArrayList<>();
-            for (String i : m.group(VersionPattern.VNUM_GROUP).split("\\."))
-                version.add(Integer.parseInt(i));
+            String[] split = m.group(VersionPattern.VNUM_GROUP).split("\\.");
+            Integer[] version = new Integer[split.length];
+            for (int i = 0; i < split.length; i++) {
+                version[i] = Integer.parseInt(split[i]);
+            }
 
             Optional<String> pre = Optional.ofNullable(
                     m.group(VersionPattern.PRE_GROUP));
@@ -1158,7 +1177,7 @@
                         + " build or optional components: '" + s + "'");
                 }
             }
-            return new Version(version, pre, build, optional);
+            return new Version(List.of(version), pre, build, optional);
         }
 
         private static boolean isSimpleNumber(String s) {
@@ -1269,9 +1288,7 @@
          * During this comparison, a version with optional build information is
          * considered to be greater than a version without one. </p>
          *
-         * <p> A version is not comparable to any other type of object.
-         *
-         * @param  ob
+         * @param  obj
          *         The object to be compared
          *
          * @return  A negative integer, zero, or a positive integer if this
@@ -1282,8 +1299,8 @@
          *          If the given object is {@code null}
          */
         @Override
-        public int compareTo(Version ob) {
-            return compare(ob, false);
+        public int compareTo(Version obj) {
+            return compare(obj, false);
         }
 
         /**
@@ -1294,9 +1311,10 @@
          * described in {@link #compareTo(Version)} with the exception that the
          * optional build information is always ignored. </p>
          *
-         * <p> A version is not comparable to any other type of object.
+         * <p> This method provides ordering which is consistent with
+         * {@code equalsIgnoreOptional()}. </p>
          *
-         * @param  ob
+         * @param  obj
          *         The object to be compared
          *
          * @return  A negative integer, zero, or a positive integer if this
@@ -1306,47 +1324,47 @@
          * @throws  NullPointerException
          *          If the given object is {@code null}
          */
-        public int compareToIgnoreOptional(Version ob) {
-            return compare(ob, true);
+        public int compareToIgnoreOptional(Version obj) {
+            return compare(obj, true);
         }
 
-        private int compare(Version ob, boolean ignoreOpt) {
-            if (ob == null)
-                throw new NullPointerException("Invalid argument");
+        private int compare(Version obj, boolean ignoreOpt) {
+            if (obj == null)
+                throw new NullPointerException();
 
-            int ret = compareVersion(ob);
+            int ret = compareVersion(obj);
             if (ret != 0)
                 return ret;
 
-            ret = comparePre(ob);
+            ret = comparePre(obj);
             if (ret != 0)
                 return ret;
 
-            ret = compareBuild(ob);
+            ret = compareBuild(obj);
             if (ret != 0)
                 return ret;
 
             if (!ignoreOpt)
-                return compareOptional(ob);
+                return compareOptional(obj);
 
             return 0;
         }
 
-        private int compareVersion(Version ob) {
+        private int compareVersion(Version obj) {
             int size = version.size();
-            int oSize = ob.version().size();
+            int oSize = obj.version().size();
             int min = Math.min(size, oSize);
             for (int i = 0; i < min; i++) {
                 int val = version.get(i);
-                int oVal = ob.version().get(i);
+                int oVal = obj.version().get(i);
                 if (val != oVal)
                     return val - oVal;
             }
             return size - oSize;
         }
 
-        private int comparePre(Version ob) {
-            Optional<String> oPre = ob.pre();
+        private int comparePre(Version obj) {
+            Optional<String> oPre = obj.pre();
             if (!pre.isPresent()) {
                 if (oPre.isPresent())
                     return 1;
@@ -1368,8 +1386,8 @@
             return 0;
         }
 
-        private int compareBuild(Version ob) {
-            Optional<Integer> oBuild = ob.build();
+        private int compareBuild(Version obj) {
+            Optional<Integer> oBuild = obj.build();
             if (oBuild.isPresent()) {
                 return (build.isPresent()
                         ? build.get().compareTo(oBuild.get())
@@ -1380,8 +1398,8 @@
             return 0;
         }
 
-        private int compareOptional(Version ob) {
-            Optional<String> oOpt = ob.optional();
+        private int compareOptional(Version obj) {
+            Optional<String> oOpt = obj.optional();
             if (!optional.isPresent()) {
                 if (oOpt.isPresent())
                     return -1;
@@ -1427,10 +1445,7 @@
          * <p> Two {@code Version}s are equal if and only if they represent the
          * same version string.
          *
-         * <p> This method satisfies the general contract of the {@link
-         * Object#equals(Object) Object.equals} method. </p>
-         *
-         * @param  ob
+         * @param  obj
          *         The object to which this {@code Version} is to be compared
          *
          * @return  {@code true} if, and only if, the given object is a {@code
@@ -1438,12 +1453,12 @@
          *
          */
         @Override
-        public boolean equals(Object ob) {
-            boolean ret = equalsIgnoreOptional(ob);
+        public boolean equals(Object obj) {
+            boolean ret = equalsIgnoreOptional(obj);
             if (!ret)
                 return false;
 
-            Version that = (Version)ob;
+            Version that = (Version)obj;
             return (this.optional().equals(that.optional()));
         }
 
@@ -1454,7 +1469,7 @@
          * <p> Two {@code Version}s are equal if and only if they represent the
          * same version string disregarding the optional build information.
          *
-         * @param  ob
+         * @param  obj
          *         The object to which this {@code Version} is to be compared
          *
          * @return  {@code true} if, and only if, the given object is a {@code
@@ -1462,13 +1477,13 @@
          *          ignoring the optional build information
          *
          */
-        public boolean equalsIgnoreOptional(Object ob) {
-            if (this == ob)
+        public boolean equalsIgnoreOptional(Object obj) {
+            if (this == obj)
                 return true;
-            if (!(ob instanceof Version))
+            if (!(obj instanceof Version))
                 return false;
 
-            Version that = (Version)ob;
+            Version that = (Version)obj;
             return (this.version().equals(that.version())
                 && this.pre().equals(that.pre())
                 && this.build().equals(that.build()));
@@ -1477,9 +1492,6 @@
         /**
          * Returns the hash code of this version.
          *
-         * <p> This method satisfies the general contract of the {@link
-         * Object#hashCode Object.hashCode} method.
-         *
          * @return  The hashcode of this version
          */
         @Override
@@ -1507,8 +1519,7 @@
         private static final String BUILD
             = "(?:(?<PLUS>\\+)(?<BUILD>0|[1-9][0-9]*)?)?";
         private static final String OPT      = "(?:-(?<OPT>[-a-zA-Z0-9.]+))?";
-        private static final String VSTR_FORMAT
-            = "^" + VNUM + PRE + BUILD + OPT + "$";
+        private static final String VSTR_FORMAT = VNUM + PRE + BUILD + OPT;
 
         static final Pattern VSTR_PATTERN = Pattern.compile(VSTR_FORMAT);
 
--- a/jdk/src/java.base/share/classes/java/lang/VersionProps.java.template	Thu Apr 20 22:46:22 2017 -0700
+++ b/jdk/src/java.base/share/classes/java/lang/VersionProps.java.template	Fri Apr 21 19:13:47 2017 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,7 +26,7 @@
 package java.lang;
 
 import java.io.PrintStream;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
@@ -77,21 +77,32 @@
 
     // This method is reflectively used by regression tests.
     static List<Integer> parseVersionNumbers(String version) {
-        List<Integer> verNumbers = new ArrayList<>(4);
+        // Let's find the size of an array required to hold $VNUM components
+        int size = 0;
         int prevIndex = 0;
+        do {
+            prevIndex = version.indexOf('.', prevIndex) + 1;
+            size++;
+        } while (prevIndex > 0);
+        Integer[] verNumbers = new Integer[size];
+
+        // Fill in the array with $VNUM components
+        int n = 0;
+        prevIndex = 0;
         int index = version.indexOf('.');
-        while (index > 0) {
-            verNumbers.add(parseVersionNumber(version, prevIndex, index));
+        while (index > -1) {
+            verNumbers[n] = parseVersionNumber(version, prevIndex, index);
             prevIndex = index + 1; // Skip the period
             index = version.indexOf('.', prevIndex);
+            n++;
         }
-        verNumbers.add(parseVersionNumber(version, prevIndex, version.length()));
+        verNumbers[n] = parseVersionNumber(version, prevIndex, version.length());
 
-        if (verNumbers.get(0) == 0 || verNumbers.get(verNumbers.size() - 1) == 0)
-            throw new IllegalArgumentException("Leading/trailing zeros not supported (" +
-                    verNumbers + ")");
+        if (verNumbers[0] == 0 || verNumbers[n] == 0)
+            throw new IllegalArgumentException("Leading/trailing zeros not allowed (" +
+                    Arrays.toString(verNumbers) + ")");
 
-        return verNumbers;
+        return List.of(verNumbers);
     }
 
     static List<Integer> versionNumbers() {
--- a/jdk/test/java/lang/Runtime/Version/Basic.java	Thu Apr 20 22:46:22 2017 -0700
+++ b/jdk/test/java/lang/Runtime/Version/Basic.java	Fri Apr 21 19:13:47 2017 +0100
@@ -23,19 +23,17 @@
 
 /*
  * @test
- * @summary Unit test for java.lang.Runtime.Version.
+ * @summary Unit test for java.lang.Runtime.Version
  * @bug 8072379 8144062 8161236 8160956
  */
 
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
 import java.lang.Runtime.Version;
 import java.math.BigInteger;
-import java.util.stream.Collectors;
+import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 import static java.lang.System.out;
 
@@ -46,7 +44,6 @@
         = NullPointerException.class;
     private static final Class<? extends Throwable> NFE
         = NumberFormatException.class;
-    private static final Class<?> VERSION = Version.class;
 
     private static final BigInteger TOO_BIG
         = (BigInteger.valueOf(Integer.MAX_VALUE)).add(BigInteger.ONE);
@@ -232,7 +229,7 @@
         String [] ver = jv[0].split("-");
         List<Integer> javaVerVNum
             = Arrays.stream(ver[0].split("\\."))
-            .map(v -> Integer.parseInt(v))
+            .map(Integer::parseInt)
             .collect(Collectors.toList());
         if (!javaVerVNum.equals(current.version())) {
             fail("Runtime.version()", javaVerVNum.toString(),
@@ -256,7 +253,7 @@
     }
 
     private static void testVersion(List<Integer> vnum, String s) {
-        List<Integer> svnum = new ArrayList<Integer>();
+        List<Integer> svnum = new ArrayList<>();
         StringBuilder sb = new StringBuilder();
         for (int i = 0; i < s.length(); i++) {
             Character c = s.charAt(i);
@@ -297,21 +294,20 @@
     }
 
     private static void testEqualsNO(Version v0, Version v1, boolean eq) {
-        if ((eq && !v0.equalsIgnoreOptional(v1))
-            || (!eq && v0.equalsIgnoreOptional(v1))) {
+        if (eq == v0.equalsIgnoreOptional(v1)) {
+            pass();
+        } else {
             fail("equalsIgnoreOptional() " + Boolean.toString(eq),
                  v0.toString(), v1.toString());
-        } else {
-            pass();
         }
     }
 
     private static void testEquals(Version v0, Version v1, boolean eq) {
-        if ((eq && !v0.equals(v1)) || (!eq && v0.equals(v1))) {
+        if (eq == v0.equals(v1)) {
+            pass();
+        } else {
             fail("equals() " + Boolean.toString(eq),
                  v0.toString(), v1.toString());
-        } else {
-            pass();
         }
     }
 
@@ -329,41 +325,24 @@
         }
     }
 
-    private static void testCompareNO(Version v0, Version v1, int compare)
-    {
-        try {
-            Method m = VERSION.getMethod("compareToIgnoreOptional", VERSION);
-            int cmp = (int) m.invoke(v0, v1);
-            checkCompare(v0, v1, compare, cmp);
-        } catch (IllegalAccessException | InvocationTargetException |
-                 NoSuchMethodException ex) {
-            fail(String.format("compareToIgnoreOptional() invocation: %s",
-                               ex.getClass()),
-                 null);
-        }
+    private static void testCompareNO(Version v0, Version v1, int compare) {
+        int cmp = v0.compareToIgnoreOptional(v1);
+        checkCompare(v0, v1, compare, cmp);
     }
 
     private static void testCompare(Version v0, Version v1, int compare) {
-        try {
-            Method m = VERSION.getMethod("compareTo", VERSION);
-            int cmp = (int) m.invoke(v0, v1);
-            checkCompare(v0, v1, compare, cmp);
-        } catch (IllegalAccessException | InvocationTargetException |
-                 NoSuchMethodException ex) {
-            fail(String.format("compareTo() invocation: %s", ex.getClass()),
-                 null);
-        }
+        int cmp = v0.compareTo(v1);
+        checkCompare(v0, v1, compare, cmp);
     }
 
     private static void checkCompare(Version v0, Version v1,
-                                     int compare, int cmp)
+                                     int expected, int actual)
     {
-        if (((cmp == 0) && (compare == 0))
-            || (compare == (cmp / Math.abs(cmp == 0 ? 1 : cmp)))) {
+        if (Integer.signum(expected) == Integer.signum(actual)) {
             pass();
         } else {
-            fail(String.format("compare() (cmp = %s) (compare = %s)",
-                               cmp, compare),
+            fail(String.format("compare() (actual = %s) (expected = %s)",
+                               actual, expected),
                  v0.toString(), v1.toString());
         }
     }