RFR [9] 8177738: Runtime.Version must be a value-based class
Paul Sandoz
paul.sandoz at oracle.com
Mon Apr 3 19:39:28 UTC 2017
Hi,
962 * [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
You removed the initial “^” which is still mentioned in JDK-8148822
< `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
---
> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary
JEP 223 is still using the former. Which is correct?
Since Version is now value based i think you need to check where it’s identity is used and replace with equals (which means ensuring that equals is efficient and could use identity under the covers). See usages in JarFile.
—
Separately i wish we could avoid using List<Integer> as the internal representation, it’s very inefficient, int[] is much better, the cost is then on the version() method but it’s easy to create an immutable List wrapping the array. (I logged an issue for that, but cannot remember the number.)
You patch gets us closer to that, especially with the change to parseVersionNumbers method.
Paul.
> On 30 Mar 2017, at 10:04, Pavel Rappo <pavel.rappo at oracle.com> wrote:
>
> Hello,
>
> Please review the following change:
>
> http://cr.openjdk.java.net/~prappo/8177738/webrev.00/
>
> This is an attempt to resolve a group of issues related to Version-String API
> and its implementation:
>
> 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
>
> A good amount of work has been already done here and we just need to finish it.
> Let me explain what this patch does starting from the most trivial changes.
>
> - Fixes certain javadoc issues [a] (e.g. dangling html tags, overly qualified
> symbol references in @link tags, etc.)
>
> - Refactors Basic.java test, making it a little bit easier to read/modify this
> test in the future.
>
> - Addresses [2] in Runtime.java:963,1050. In addition I've rephrased Runtime.java:951
> to reflect the fact a version string contains only the mentioned components and
> nothing else. In other words, $VSTR *matches* a valid version-string rather than
> *finds* itself somewhere in this version-string.
>
> - Makes Runtime.Version a value-based class [1][5]. The class has gotten `final`
> modifier, the only constructor has become private and the usual (in this case)
> verbiage has been added to the javadoc. Before that, Runtime.Version had ticked
> all other the boxes in the value-based class checklist already.
>
> - Improves $VNUM list mechanics in VersionProps.java.template and Runtime.java:1149-1153
> by providing immutable list instead of unmodifiable. This change has become
> possible due to the fact Runtime.Version constructor has become private and,
> thus, all the callers are known and trusted.
>
> - Removes redundant/explicit javadoc declarations in `hashCode`, `equals` and
> `compareTo` methods. These methods are documented needlessly explicit. The only
> truly justified comment sits inside `compareToIgnoreOptional` method.
> Since `compareToIgnoreOptional` and `equalsIgnoreOptional` are not inherited,
> I marked the "consistent-with" relationship between them explicitly [c].
>
> - Now a few words about a not-so-pleasant part which addresses [3][4]. In a
> nutshell the regex-style format ($VSTR) which is used in the spec to define the
> version-string, matches all valid version strings and *some more*.
>
> Here's the format:
>
> $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?
>
> If we expand all optional elements we will get the following 8 possibilities:
>
> ## $VSTR $PRE $BUILD $OPT EXAMPLE
> --------------------------------------------------------------------------------
> 1 $VNUM 0 0 0 9.0.1
> 2 $VNUM+-$OPT 0 0 1 9.0.1+-longcat.dev
> 3 $VNUM+$BUILD 0 1 0 9.0.1+256
> 4 $VNUM+$BUILD-$OPT 0 1 1 9.0.1+256-longcat.dev
> 5 $VNUM-$PRE 1 0 0 9.0.1-ea
> * 6 $VNUM-$PRE+-$OPT 1 0 1 9.0.1-ea+-longcat.dev
> 7 $VNUM-$PRE+$BUILD 1 1 0 9.0.1-ea+256
> 8 $VNUM-$PRE+$BUILD-$OPT 1 1 1 9.0.1-ea+256-longcat.dev
>
> Now, ##6 was disallowed by design. In the case when $BUILD is not present, but
> both $PRE and $OPT are, the $VSTR will instead look like this:
>
> 6 $VNUM-$PRE-$OPT 1 0 1 9.0.1-ea-longcat.dev
>
> (i.e. without the "+" separator).
>
> This is a problem because it means that in addition to the format it needs a
> clear description of exceptional cases which are to be subtracted from the set
> of matched version-strings. Such a description has been proposed in [6] (you can
> have a look at the webrev that thread refers to):
>
> * <p> If the build number is unset, then {@code '+'} may only be present
> * when {@code $OPT} is present and {@code $PRE} is not.
>
> It indeed may be a satisfactory solution. However I would argue that it's
> probably not the best one. I suspect spec readers will jump straight to the
> format completely missing "the special case" paragraph simply because the format
> attracts attention and neatly defines the version-string. Neatly but wrongly.
>
> Another option would be to split the $VSTR format into a minimal number of
> sub-cases (e.g. Runtime:1032-1034). This will require no extra "special case"
> paragraph and will draw reader's attention to all the subtleties on the spot.
>
> # ##
> --------------------------------------------------------------------------------
> 1 $VNUM(-$PRE)?\+$BUILD(-$OPT)? 3, 4, 7, 8
> 2 $VNUM-$PRE(-$OPT)? 5, 6
> 3 $VNUM(+-$OPT)? 1, 2
>
> One more option would be to require "+" to be there in the case where any
> component to the right from $PRE is present (i.e. $BUILD or $OPT or both).
> This will greatly simplify the rules and allow the $VSTR to stay as it is today.
>
> This option would affect only the case ##6:
>
> 6 $VNUM-$PRE+-$OPT 1 0 1 9.0.1-ea+-longcat.dev
> ^^
> Yes, it looks a bit ugly, but we already have this ugliness in case 2 (and we
> can't get rid of it because of [3]):
>
> 2 $VNUM+-$OPT 0 0 1 9.0.1+-longcat.dev
> ^^
> I suspect both 2 and 6 will be pretty rare anyway as it would mean optional
> build information is there, but the build number is not.
> Sounds like a corner case to me.
>
> Thanks,
> -Pavel
>
> --------------------------------------------------------------------------------
> [1] https://bugs.openjdk.java.net/browse/JDK-8177738
> [2] https://bugs.openjdk.java.net/browse/JDK-8148822
> [3] https://bugs.openjdk.java.net/browse/JDK-8148877
> [4] https://bugs.openjdk.java.net/browse/JDK-8160954
> [5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046813.html
> [6] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html
>
> [a] If you build locally or use promoted builds, please keep in mind there's a
> known issue with rendering documentation for Runtime.Version in
> some browsers: https://bugs.openjdk.java.net/browse/JDK-8177727
> [b] Startup time/memory footprint might benefit as well (it will definitely
> require some benchmarking to determine if it is the case)
> [c] Even though I'm fully aware I do not have the full knowledge the design
> decisions were based upon, I must say I fail to see why equalsIgnoreOptional
> and compareToIgnoreOptional methods have been introduced from the day 1.
> It looks like something that could be neatly solved by providing a custom
> Comparator if needed.
>
More information about the core-libs-dev
mailing list