[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
Date: Fri, 3 Mar 2017 14:01:53 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

On 03/03/2017 01:50 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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 <address@hidden>
>>> ---
>> 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: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]