Re: [PATCH v2 15/28] qapi misc: Elide redundant has_FOO in generated C

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> The has_FOO for pointer-valued FOO are redundant, except for arrays.
>> >> They are also a nuisance to work with.  Recent commit "qapi: Start to
>> >> elide redundant has_FOO in generated C" provided the means to elide
>> >> them step by step.  This is the step for qapi/misc.json.
>> >> 
>> >> Said commit explains the transformation in more detail.  The invariant
>> >> violations mentioned there do not occur here.
>> >> 
>> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

>> >> diff --git a/util/qemu-config.c b/util/qemu-config.c
>> >> index 5325f6bf80..95f61fc883 100644
>> >> --- a/util/qemu-config.c
>> >> +++ b/util/qemu-config.c
>> >> @@ -80,14 +80,8 @@ static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
>> >>              break;
>> >>          }
>> >>  
>> >> -        if (desc[i].help) {
>> >> -            info->has_help = true;
>> >> -            info->help = g_strdup(desc[i].help);
>> >> -        }
>> >> -        if (desc[i].def_value_str) {
>> >> -            info->has_q_default = true;
>> >> -            info->q_default = g_strdup(desc[i].def_value_str);
>> >> -        }
>> >> +        info->help = g_strdup(desc[i].help);
>> >> +        info->q_default = g_strdup(desc[i].def_value_str);
>> >>  
>> >>          QAPI_LIST_PREPEND(param_list, info);
>> >>      }
>> >> @@ -245,8 +239,7 @@ static QemuOptsList machine_opts = {
>> >>      }
>> >>  };
>> >>  
>> >> -CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>> >> -                                                          const char *option,
>> >> +CommandLineOptionInfoList *qmp_query_command_line_options(const char *option,
>> >>                                                            Error **errp)
>> >>  {
>> >>      CommandLineOptionInfoList *conf_list = NULL;
>> >> @@ -254,7 +247,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
>> >>      int i;
>> >>  
>> >>      for (i = 0; vm_config_groups[i] != NULL; i++) {
>> >> -        if (!has_option || !strcmp(option, vm_config_groups[i]->name)) {
>> >> +        if (!option || !strcmp(option, vm_config_groups[i]->name)) {
>> >
>> > I think that can be g_strcmp0 if you can convince yourself ->name is
>> > non-null
>> 
>> vm_config_groups[i] must not be null.
>> 
>> However, replacing the whole condition by !g_strcmp0() would be wrong:
>> 
>>     option                             |  null   ->name  neither
>>     -----------------------------------+------------------------
>>     !option || !strcmp(option, ->name) |  true     true    false
>>     g_strcmp0(option, ->name)          | false     true    false
>
> Oops yes, sorry these are the other way around as you point out.

I fell into the exact same trap myself :)

[...]