On 2016-06-27 22:11, Iris Clark wrote:
Hi, Claes.
[ Sorry for the premature send, my keyboard started interpreting my shortcuts
in unexpected ways. ]
http://cr.openjdk.java.net/~redestad/8160000/webrev.5/
Nice bugid.
I can hardly believe my luck! :-)
Overall, this change looks good. I just have a few concerns.
Have you built this forcing alternative build numbers using the makefile
"--with-version-<...>" options? For JDK 9 builds, the default version number
is only "9". You should make sure that versions "9.0.X" and "9.0.0.X" for
arbitrary value of X work as expected.
Done.
Runtime.java: I wonder if unmodifiablelist() should be invoked at new line
1090 instead of 941? Depending on the caller to provide an unmodifiable list
without additional verification seems like a potential problem. If you move
the invocation, then you may want to consider reducing the arguments of
Version() at lines 1086-1089 to the single VersionProps.
I expect to address the following outstanding issues in the coming weeks. I
don't think that they will impact your changes, but I leave it for you to
judge:
8156711: Runtime.Version.version should be an int[] instead of a
List<Integer>
https://bugs.openjdk.java.net/browse/JDK-8156711
This is an implementation change that was suggested during review. The
thought was that using an int[] is more efficient for space and comparisons.
If you believe that there is benefit to adding this change to yours, feel free
to take on the bug. Otherwise, assuming that there is still benefit to making
this change, I'll take care of it after your changes are in a promotion.
Seems trivial enough, but I think the benefit of this is marginal at
best unless we expect Runtime.Version to be used extensively in
performance critical code. For startup it may even be an ever so slight
negative if it means more code to parse the built-in version string.
8156907: Runtime.Version.{major(),security()} return value of 0 may be
ambiguous
https://bugs.openjdk.java.net/browse/JDK-8156907
After careful consideration, this issue will be resolved by changing the
parse() implementation to allow trialing zero elements (now in
VersionBuilder.parse()). As an example of the target behavior,
v = Runtime.Version.parse("9.0.0") will not throw an IAE and v.toString()
will return "9". Specification adjustments will be needed. Since
VersionBuilder does not change the parse() implementation, I believe that
there won't be any problems.
Good.
I don't think there'll be any conflict with this change, and as I was
testing I also made sure that X=0 works well since the build scripts
already handles shortening this to "9".
To accommodate your change request w.r.t. unmodifiableList above I took
the liberty of cleaning up VersionBuilder.parse() a bit, though. Hope
you don't mind:
http://cr.openjdk.java.net/~redestad/8160000/webrev.6/
Thanks!
/Claes
Thanks,
iris
