Re: [Qemu-devel] [PATCH 3/9] QMP: First half of the new argument checking code

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 02 Jun 2010 09:22:40 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> Higher order functions rock.  But C is too static and limited for
>> elegant use of higher order functions.  Means to construct loops are
>> usually more convenient to use, and yield more readable code.
>> 
>> I find the use of qdict_iter() here quite tortuous: you define a
>> separate iterator function, which you can't put next to its use.  You
>> need to jump back and forth between the two places to understand what
>> the loop does.  You define a special data structure just to pass
>> arguments and results through qdict_iter().
>> 
>> Let me try to sketch the alternative:
>> 
>> static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
>> {
>>     QDict *cmd_args;
>>     int res = 0, skip = 0;
>>     QDictEntry *ent;
>> 
>>     cmd_args = qdict_from_args_type(cmd->args_type);
>> 
>>     for (ent = qdict_first_entry(cmd_args); ent; ent = qdict_next_entry(ent) {
>
>  I thought about doing something similar a while ago, but I dislike it for
> two reasons:
>
>   1. I don't think the notion of 'first' and 'next' apply for dicts. One may
>      argue that the iterator has the same issue, but it's implicit

Does the dirt under the carpet exist when nobody looks?

Iterating over an unordered collection necessarily creates an order
where none was before.  It's the nature of iteration.  Dressing it up as
iterator + function argument doesn't change the basic fact[*].

>   2. QDictEntry shouldn't be part of the public interface, we should be
>      using forward declaration there

No problem, just add qdict_ent_key() and qdict_ent_value(), and use them
instead of operator ->.

>                                      (although I'm not sure whether this is
>      possible with a typedef)

In qdict.h: typedef struct QDictEntry QDictEntry;

In qdict.c: struct QDictEntry { ... };

>  I think having qdict_foreach() will improve things already.

I doubt it, but try and see :)


[*] Unless the iterator gets fancy and calls the function argument
concurrently.  Hardly an option in primitive old C.