Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]

On 03/03/2017 01:50 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>>> Fix the design flaw demonstrated in the previous commit: new method
>>> check_list() lets input visitors report that unvisited input remains
>>> for a list, exactly like check_struct() lets them report that
>>> unvisited input remains for a struct or union.
>>>
>>> Implement the method for the qobject input visitor (straightforward),
>>> and the string input visitor (less so, due to the magic list syntax
>>> there).  The opts visitor's list magic is even more impenetrable, and
>>> all I can do there today is a stub with a FIXME comment.  No worse
>>> than before.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>> Didn't I already review this one?
>>
>> Ah, there's my R-b:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html
> 

>>> 
>>> --- a/qapi/qobject-input-visitor.c
>>> +++ b/qapi/qobject-input-visitor.c
>>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>>      return container_of(v, QObjectInputVisitor, visitor);
>>>  }
>>>  
>>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>>> +                                 int n)
>>>  {

No function comment, so the _nth and int n are guesses on their meaning...


>> If I'm reading this right, your use of n-- in the loop followed by the
>> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
>> lets see what callers pass for n:
> 
> At least @n times.

Ah, as in 'use first available result' or 'iterate at least once', based
on our callers, but could also mean 'iterate at least twice' for a
caller that passes 2.


>> the other passes 1.  No other calls.  Did we really need an integer,
>> where we use n--, or would a bool have done as well?
> 
> Since I actually use only 0 and 1, a bool would do, but would it make
> the code simpler?

I don't know that a bool would be any simpler,

> 
>> At any rate, since I've already reviewed it once, you can add R-b, but
>> we may want a followup to make it less confusing.
> 
> Would renaming the function to full_name_but_n() help?

Or even keep the name unchanged, but add function comments describing
what 'n' means.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]