On Wed, 7 May 2025 02:33:50 GMT, Ashutosh Mehra <asme...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @vnkozlov comments
>
> src/hotspot/share/cds/cdsConfig.cpp line 428:
>
>> 426:
>> 427: // At least one AOT flag has been used
>> 428: _new_aot_flags_used = true;
>
> indentation seems off?
Fixed.
> src/hotspot/share/cds/cdsConfig.cpp line 490:
>
>> 488: bool has_output = !FLAG_IS_DEFAULT(AOTCacheOutput);
>> 489:
>> 490: if (has_output) {
>
> Can this if-else block be refactored as:
>
>
> if (!has_output && !has_config) {
> vm_exit_during_initialization("...");
> }
> if (has_output) {
>
> }
>
> This pattern in much easier to read IMO.
Fixed.
> src/hotspot/share/cds/cdsConfig.cpp line 504:
>
>> 502: } else {
>> 503: if (!has_config) {
>> 504: vm_exit_during_initialization("-XX:AOTMode=record cannot be used
>> without setting AOTCacheOutput or AOTConfiguration");
>
> I suggest to be consistent in phrasing the error messages related to
> incomplete options. I liked the way it is worded in `check_aotmode_create()`.
> So my preference is to replace `-XX:AOTMode=record cannot be used without
> setting AOTCacheOutput or AOTConfiguration` with `AOTCacheOutput or
> AOTConfiguration must be specified when using -XX:AOTMode=record`. But you
> may chose otherwise as long as the structure of the error messages is
> consistent.
I changed to "At least one of AOTCacheOutput and AOTConfiguration must be
specified when using -XX:AOTMode=record", so it's clear that both option can be
specified together.
> src/hotspot/share/cds/cdsConfig.cpp line 526:
>
>> 524: void CDSConfig::check_aotmode_create() {
>> 525: if (FLAG_IS_DEFAULT(AOTConfiguration)) {
>> 526: vm_exit_during_initialization("-XX:AOTMode=create cannot be used
>> without setting AOTConfiguration");
>
> Same suggestion regarding error msg:
> Replace `-XX:AOTMode=create cannot be used without setting AOTConfiguration`
> with
> `AOTConfiguration must be specified when using -XX:AOTMode=create`
Fixed.
> src/hotspot/share/cds/metaspaceShared.cpp line 1082:
>
>> 1080: for (int i = 0; i < Arguments::num_jvm_args(); i++) {
>> 1081: const char* arg = Arguments::jvm_args_array()[i];
>> 1082: if (strncmp("-XX:AOTCacheOutput=", arg, 19) == 0 ||
>
> I have seen this hard coding of string length in the `strncmp` call many
> times in the code base. I wonder why we don't use something like this:
>
> strncmp("-XX:AOTCacheOutput=", arg, sizeof("-XX:AOTCacheOutput=")-1)
>
> Counting chars is really error prone. sizeof("...")-1 gives the same result.
I end up using `strstr()`, as I don't want to repeat the string literal twice
(risking a typo) or use a macro.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796934
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796911
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796869
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796827
PR Review Comment: https://git.openjdk.org/jdk/pull/24942#discussion_r2076796802
