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=14181=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14181=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 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
