On Tue, 28 Nov 2023 23:24:53 GMT, Ioi Lam <ik...@openjdk.org> wrote:
> This is a simple clean up that moves the code for initializing the CDS config
> states from arguments.cpp to cdsConfig.cpp
>
> I renamed a few functions, but otherwise the code is unchanged.
>
> - `get_default_shared_archive_path()` -> `default_archive_path()`
> - `GetSharedArchivePath()` -> `static_archive_path()`
> - `GetSharedDynamicArchivePath()` -> `dynamic_archive_path()`
>
> There's also less `#if INCLUDE_CDS` since the entire cdsConfig.cpp file is
> compiled only if CDS is enabled.
This looks good! I think this is a good opportunity to refactor some of the
code for better readability so I left some comments below.
src/hotspot/share/cds/cdsConfig.cpp line 101:
> 99: void CDSConfig::extract_shared_archive_paths(const char* archive_path,
> 100: char** base_archive_path,
> 101: char** top_archive_path) {
Could you align these arguments?
src/hotspot/share/cds/cdsConfig.cpp line 125:
> 123: }
> 124:
> 125: void CDSConfig::init_shared_archive_paths() {
Now that I see this there is a lot of indentation thanks to the nested
conditionals. I don't have much to offer but is there a cleaner way to format
this method? Maybe you can extract the code in `if (archives == 1)` into its
own method for better readability.
src/hotspot/share/runtime/arguments.cpp line 1262:
> 1260: }
> 1261:
> 1262: CDSConfig::check_system_property(key, value);
I see this is only called once, do you expect this method to be used again? It
may be unnecessary to extract this code into its own method.
-------------
PR Review: https://git.openjdk.org/jdk/pull/16868#pullrequestreview-1760626242
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412613074
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412616593
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412610795
