Re: [Qemu-devel] [PATCH 02/16] qapi: Drop superfluous qapi_enum_parse() parameter max

[Eric Blake]

On 08/24/2017 01:35 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 08/24/2017 03:45 AM, Markus Armbruster wrote:
>>> The lookup tables have a sentinel, no need to make callers pass their
>>> size.
>>>
>>> Fun: the header has it in the wrong position.  Good riddance.
>>>

>>> +++ b/include/qapi/util.h
>>> @@ -12,7 +12,7 @@
>>>  #define QAPI_UTIL_H
>>>  
>>>  int qapi_enum_parse(const char * const lookup[], const char *buf,
>>> -                    int max, int def, Error **errp);
>>> +                    int def, Error **errp);
>>
>> I'm not sure what you meant by wrong position; were you thinking that
>> lookup/max should be immediately adjacent (since max is a property of
>> the lookup[] parameter), and sticking 'buf' in between the two is what
>> meant 'max' was in the wrong position?
> 
> Compare the declaration above with the definition below:
> 
>     diff --git a/qapi/qapi-util.c b/qapi/qapi-util.c
>     index 46eda7d..ee7594f 100644
>     --- a/qapi/qapi-util.c
>     +++ b/qapi/qapi-util.c
>     @@ -16,7 +16,7 @@
>      #include "qapi/util.h"
> 
>      int qapi_enum_parse(const char * const lookup[], const char *buf,
>     -                    int max, int def, Error **errp)
>     +                    int def, Error **errp)
>      {
>          int i;
> 
> Declaration has max before def, definition has it the other way round.

Huh?  On current master (commit 248b2373), the two look like they match
to me:

$ git grep -A1 qapi_enum_parse'.*const look'
include/qapi/util.h:int qapi_enum_parse(const char * const lookup[],
const char *buf,
include/qapi/util.h-                    int max, int def, Error **errp);
--
qapi/qapi-util.c:int qapi_enum_parse(const char * const lookup[], const
char *buf,
qapi/qapi-util.c-                    int max, int def, Error **errp)


> 
> Such errors are one reason I prefer to have documentation next to
> definitions, which are authoritative, rather than declarations, which
> may or may not match the definition.
> 
>> The change itself is reasonable, even if the commit message needs a
>> tweak to answer my question.
> 
> Care to suggest a wording?

At this point, I find the claim to be bogus, so I suggest you delete the
'Fun:' paragraph.

> 
>> Reviewed-by: Eric Blake <address@hidden>
> 
> Thanks!
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature