Re: [Qemu-devel] [PATCH v16 11/24] qmp-input: Refactor when list is advanced

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 04/29/2016 02:50 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> In the QMP input visitor, visiting a list traverses two objects:
>>> the QAPI GenericList of the caller (which gets advanced in
>>> visit_next_list() regardless of this patch), and the QList input
>>> that we are converting to QAPI.  For consistency with QDict
>>> visits, we want to consume elements from the input QList during
>>> the visit_type_FOO() for the list element; that is, we want ALL
>>> the code for consuming an input to live in qmp_input_get_object(),
>>> rather than having it split according to whether we are visiting
>>> a dict or a list.  Making qmp_input_get_object() the common point
>>> of consumption will make it easier for a later patch to refactor
>>> visit_start_list() to cover the GenericList * head of a QAPI list,
>>> and in turn will get rid of the 'first' flag (which lived in
>>> qmp_input_next_list() pre-patch, and is hoisted to StackObject
>>> by this patch).
>>>
>>> This patch is therefore shifting the post-condition use of
>>> 'entry' from:
>>>
>>>         start_list next_list type_ELT ... next_list type_ELT next_list 
>>> end_list
>>>  visit                       1st elt                last elt
>>>  entry  NULL       1st elt   1st elt      last elt  last elt NULL      gone
>> 
>> You added a line "visit", but the introduction still only mentions
>> 'entry'.  Mildly confusing.  If you have an idea on how to improve it, I
>> can touch it up on commit.
>
> [probably word wrapped by my mailer, but how about]
>
> This patch is therefore altering the post-condition use of 'entry',
> while keeping what gets visited unchanged, from:
>
>          start_list next_list type_ELT ... next_list type_ELT next_list
> end_list
>   visits                      1st elt                last elt
>   entry  NULL       1st elt   1st elt      last elt  last elt NULL      gone

Sold.

>>>
>>> where type_ELT() returns (entry ? entry : 1st elt) and next_list() steps
>>> entry
>>>
>>> to this usage:
>>>
>>>         start_list next_list type_ELT ... next_list type_ELT next_list 
>>> end_list
>>>  visit                       1st elt                last elt
>
> and another s/visit/visits/ here
>
>>>  entry  1st elt    1nd elt   2nd elt      last elt  NULL     NULL      gone
>>>
>>> where type_ELT() steps entry and returns the old entry, and next_list()
>>> leaves entry alone.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>> 
>> Patch looks good.
>
> I added the visit line to make it a little easier to see that type_ELT
> is still visiting the same thing, even though 'entry' ends up on a
> different spot (easier than deciphering "type_ELT() ... returns the old
> entry").  But if you don't think it adds anything, I'm also okay with
> dropping the 'visit[s]' line and sticking to just the phrase after the
> diagram for that explanation.

Either way works for me, so I'm sticking to your later version, as
amended here.