Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes [v2]
On Fri, 26 May 2023 16:54:42 GMT, Roger Riggs wrote: >> Decouple the jdk.internal.util OperatingSystem and Version classes to >> simplify class loading and avoid an indirect cyclic initialization. >> >> Move the method to get the current OS version from OperatingSystem to the >> Version class and its initialization. >> Revert to using String.toUpperCase() instead of custom code to uppercase. >> >> Update the uses of OperatingSystem.version to be Version.current. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Rename jdk.internal.util.Version to OSVersion Thanks for renaming the class. The renaming LGTM. - PR Review: https://git.openjdk.org/jdk/pull/14181#pullrequestreview-1449236628
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes [v2]
> Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Rename jdk.internal.util.Version to OSVersion - Changes: - all: https://git.openjdk.org/jdk/pull/14181/files - new: https://git.openjdk.org/jdk/pull/14181/files/f6332cd9..22e022c0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14181&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14181&range=00-01 Stats: 298 lines in 5 files changed: 135 ins; 136 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/14181.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14181/head:pull/14181 PR: https://git.openjdk.org/jdk/pull/14181
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 16:16:58 GMT, Mandy Chung wrote: > Alternatively, move `Version` record as a nested class in `OperatingSystem`. That would work WRT naming, but I thought the point was to not have to load OperatingSystem in order to load Version? Unless loading a static inner class doesn't force the loading of the outer class? - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564631561
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. For the time being, I think they should be independent and clearly named. The nesting creates a very unwieldy and long name. (Not withstanding imports) - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564640242
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. I mean static member class of `OperatingSystem`. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564637892
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 15:32:20 GMT, Roger Riggs wrote: > Should we take this opportunity to rename `jdk.internal.util.Version` into > something like `OsVersion` to make it more clear what its `current()` method > actually returns? Alternatively, move `Version` record as a nested class in `OperatingSystem`. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564625922
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. Looks good. Thanks for fixing this. It will allow ClassFileDumper to use `Path`. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14181#pullrequestreview-1446540821
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. Should we take this opportunity to rename `jdk.internal.util.Version` into something like `OsVersion` to make it more clear what its `current()` method actually returns? - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564553174
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 15:21:42 GMT, Daniel Fuchs wrote: > Should we take this opportunity to rename `jdk.internal.util.Version` into > something like `OsVersion` to make it more clear what its `current()` method > actually returns? ok, it will give a bit more flavor; there are already multiple version classes with little to distinguish them. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564567712
RFR: 8308960: Decouple internal Version and OperatingSystem classes
Decouple the jdk.internal.util OperatingSystem and Version classes to simplify class loading and avoid an indirect cyclic initialization. Move the method to get the current OS version from OperatingSystem to the Version class and its initialization. Revert to using String.toUpperCase() instead of custom code to uppercase. Update the uses of OperatingSystem.version to be Version.current. - Commit messages: - 8308960: Decouple internal Version and OperatingSystem classes Changes: https://git.openjdk.org/jdk/pull/14181/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14181&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8308960 Stats: 58 lines in 4 files changed: 24 ins; 29 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14181.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14181/head:pull/14181 PR: https://git.openjdk.org/jdk/pull/14181
