[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/16] qapi: Drop superfluous qapi_enum_parse()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 02/16] qapi: Drop superfluous qapi_enum_parse() parameter max |
Date: |
Thu, 24 Aug 2017 21:24:05 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Eric Blake <address@hidden> writes:
> 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.
Sure.
>>> Reviewed-by: Eric Blake <address@hidden>
>>
>> Thanks!
- [Qemu-devel] [PATCH 01/16] qapi: Update qapi-code-gen.txt examples to match current code, (continued)
[Qemu-devel] [PATCH 07/16] block: Use qemu_enum_parse() in blkdebug_debug_breakpoint(), Markus Armbruster, 2017/08/24
[Qemu-devel] [PATCH 03/16] tpm: Clean up driver registration & lookup, Markus Armbruster, 2017/08/24
[Qemu-devel] [PATCH 09/16] crypto: Use qapi_enum_parse() in qcrypto_block_luks_name_lookup(), Markus Armbruster, 2017/08/24
[Qemu-devel] [PATCH 15/16] qapi: Change data type of the FOO_lookup generated for enum FOO, Markus Armbruster, 2017/08/24
[Qemu-devel] [PATCH 06/16] hmp: Use qapi_enum_parse() in hmp_migrate_set_parameter(), Markus Armbruster, 2017/08/24
[Qemu-devel] [PATCH 13/16] qapi: Mechanically convert FOO_lookup[...] to FOO_str(...), Markus Armbruster, 2017/08/24