RFR: 8334492: DiagnosticCommands (jcmd) should accept %p in output filenames and substitute PID [v5]

Johan Sjölen jsjolen at openjdk.org
Sat Jul 20 11:22:32 UTC 2024


On Fri, 19 Jul 2024 19:17:54 GMT, Leonid Mesnik <lmesnik at openjdk.org> wrote:

>> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Missing copyright header update
>
> src/hotspot/share/prims/wbtestmethods/parserTests.cpp line 132:
> 
>> 130:    } else if (strcmp(type, "FILE") == 0) {
>> 131:      DCmdArgument<FileArgument> *argument =
>> 132:       new DCmdArgument<FileArgument>(name, desc, "FILE", mandatory);
> 
> Please check indentation.

On top: We hug the `*`s next to the type in Hotspot, not next to the var name. So `DCmdArgument<FileArgument>* argument`. This is something to check for all new code.

Pre-existing: The indentation of the if-block is wrong.

Also, @SoniaZaldana, would you mind changing the code to this (does not include your change), the repetition just made me cringe 😅.

```c++
  DCmdArgument<char*>* argument = nullptr;
  if (strcmp(type, "STRING") == 0) {
    argument = new DCmdArgument<char*>(name, desc, "STRING", mandatory, default_value);
  } else if (strcmp(type, "NANOTIME") == 0) {
    DCmdArgument<NanoTimeArgument>* argument = new DCmdArgument<NanoTimeArgument>(name, desc, "NANOTIME", mandatory, default_value);
  } else if (strcmp(type, "JLONG") == 0) {
    DCmdArgument<jlong>* argument = new DCmdArgument<jlong>(name, desc, "JLONG", mandatory, default_value);
  } else if (strcmp(type, "BOOLEAN") == 0) {
    DCmdArgument<bool>* argument = new DCmdArgument<bool>(name, desc, "BOOLEAN", mandatory, default_value);
  } else if (strcmp(type, "MEMORYSIZE") == 0) {
    DCmdArgument<MemorySizeArgument>* argument = new DCmdArgument<MemorySizeArgument>(name, desc, "MEMORY SIZE", mandatory, default_value);
  } else if (strcmp(type, "STRINGARRAY") == 0) {
    DCmdArgument<StringArrayArgument*>* argument = new DCmdArgument<StringArrayArgument*>(name, desc, "STRING SET", mandatory);
  }

  if (argument != nullptr) {
    if (isarg) {
      parser->add_dcmd_argument(argument);
    } else {
      parser->add_dcmd_option(argument);
    }
  }

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685384131


More information about the serviceability-dev mailing list