I’ve updated the webrev to address the issue of the constructor accepting values like Version.parse(“7.1”)
http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/ <http://cr.openjdk.java.net/~sdrach/8150680/webrev.01/> > On Jun 15, 2016, at 8:56 AM, Steve Drach <steve.dr...@oracle.com> wrote: > >>> Please review the following changeset: >>> >>> webrev: http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html >>> <http://cr.openjdk.java.net/~sdrach/8150680/webrev.00/index.html> >>> issue: https://bugs.openjdk.java.net/browse/JDK-8150680 >>> <https://bugs.openjdk.java.net/browse/JDK-8150680> >>> >>> The issue calls for reconsidering the JarFile.Release enum. A comment in >>> the bug report suggests replacing JarFile.Release with Runtime.Version, and >>> that’s what I did. Specifically I removed the enum, changed the >>> constructor to accept a Runtime.Version object instead of a JarFile.Release >>> object, updated all places in the JDK that invoked the constructor and >>> updated all tests. >>> >> Moving to Runtime.Version seems right but doesn't the javadoc for the >> constructor need to be updated to make it clear how it behavior when >> invoking with something like Version.parse("7.1") ? If I read the code >> correctly then this will be accepted and getVersion() will return 7.1. > > Yes, it needs to be updated and it needs to be fixed. Thanks for finding > that. > >> >> Fields or methods is another discussion point for the base and runtime >> versions. > > My thinking is, in this case fields and methods are equivalent, the method > not giving any more flexibility than a field. For example the method > JarFile.baseVersion will just return the value contained in the private final > static field BASE_VERSION. Or the public final static field BASE_VERSION can > be directly accessed. I see no advantage of a method here. But I’m willing > to be enlightened. > >> >> -Alan. >
