On Fri, 21 Apr 2023 16:44:13 GMT, Roger Riggs <rri...@openjdk.org> wrote:
> Create an internal Version record to hold and compare versions of the form > (major, minor, micro). > Add `OperatingSystem.version()` to return the version of the running OS. > Replace uses of os.version in java.base. > Subsequent PRs will apply to uses in other modules including, jdk.jlink, > jdk.jpackage, and java.desktop. src/java.base/macosx/classes/sun/nio/fs/BsdFileStore.java line 103: > 101: // fgetxattr broken on APFS prior to 10.14 > 102: return OperatingSystem.version() > 103: .compareTo(new Version(10, 14)) >= 0; The `Version(10, 14)` record should probably be stored in a static constant. src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 150: > 148: > 149: // Parse and save the current version > 150: private static Version osVersion = initVersion(); The version field should probably be `final`: Suggestion: public static Version version() { return CURRENT_VERSION; } // Parse and save the current version private static final Version CURRENT_VERSION = initVersion(); src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 157: > 155: return Version.parse(osVer); > 156: } catch (IllegalArgumentException ile) { > 157: throw new AssertionError("os.version malformed: " + osVer); This should probably preserve the `IllegalArgumentException` stack trace: Suggestion: } catch (IllegalArgumentException iae) { throw new AssertionError("os.version malformed: " + osVer, iae); And maybe throw `InternalError` instead: Suggestion: } catch (IllegalArgumentException iae) { throw new InternalError("os.version malformed: " + osVer, iae); test/jdk/jdk/internal/util/VersionTest.java line 96: > 94: Arguments.of(new Version(3, 3, 1), new Version(3, 3, 1), > 0), > 95: Arguments.of(new Version(3, 3, 1), new Version(3, 3, 0), > 1), > 96: Arguments.of(new Version(3, 3, 0), new Version(3, 3, 1), > -1) This should probably include tests for differing `major` versions as well: Suggestion: Arguments.of(new Version(2, 1), new Version(2, 1), 0), Arguments.of(new Version(2, 1), new Version(2, 0), +1), Arguments.of(new Version(2, 0), new Version(2, 1), -1), Arguments.of(new Version(3, 3, 1), new Version(3, 3, 1), 0), Arguments.of(new Version(3, 3, 1), new Version(3, 3, 0), +1), Arguments.of(new Version(3, 3, 0), new Version(3, 3, 1), -1), Arguments.of(new Version(2, 0), new Version(3, 0), -1), Arguments.of(new Version(3, 0), new Version(2, 0), +1) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13584#discussion_r1174347678 PR Review Comment: https://git.openjdk.org/jdk/pull/13584#discussion_r1174346831 PR Review Comment: https://git.openjdk.org/jdk/pull/13584#discussion_r1174345782 PR Review Comment: https://git.openjdk.org/jdk/pull/13584#discussion_r1174347449