On Fri, 1 Dec 2023 22:04:22 GMT, Matias Saavedra Silva <matsa...@openjdk.org>
wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed indentation
>
> 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?
Fixed.
> 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.
I want to keep the code change minimal while moving code from one file to
another. I'll refactor this function in a follow-on PR. That way it will be
easier to track the code history.
> 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.
I wanted to move the code from arguments.cpp to cdsConfig.cpp, so I had to put
it in a new function.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683767
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683760
PR Review Comment: https://git.openjdk.org/jdk/pull/16868#discussion_r1412683786
