On Sat, 20 Jul 2024 12:06:33 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> 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);
>> }
>> }
>
> @jdksjolen
>
>> Also, @SoniaZaldana, would you mind changing the code to this
>
> Even simpler (did not test, but you get my drift):
>
>
> #define ALL_TYPES_DO_XX(what) \
> what(char*, "STRING") \
> what(NanoTimeArgument, NANOTIME) \
> what(jlong, "JLONG")
> ... etc
>
> then
>
>
> #define XX(TYPE, NAME) \
> if (strcmp(type, NAME) == 0) { \
> DCmdArgument<TYPE>* argument = new DCmdArgument<TYPE>(name, desc, NAME,
> mandatory, mandatory, default_value); \
> }
> ALL_TYPES_DO_XX(XX)
> #undef XX
>
>
> ;-)
Sonia, my bad if you already know this stuff but since it's fairly esoteric
knowledge nowadays I'd like to help you out in advance: Thomas is proposing the
usage of a X macro https://en.wikipedia.org/wiki/X_macro
These can be found throughout Hotspot, you can find an example definition and
usage in `logTag.hpp` and `logTag.cpp`.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20198#discussion_r1685688287
