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

[Markus Armbruster]

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.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  block.c                  |  1 -
>>  block/file-posix.c       |  7 +++----
>>  block/file-win32.c       |  2 +-
>>  block/gluster.c          |  6 ++----
>>  block/parallels.c        |  3 ++-
>>  block/qcow2.c            |  6 ++----
>>  blockdev.c               |  1 -
>>  hmp.c                    |  2 +-
>>  include/qapi/util.h      |  2 +-
>>  migration/global_state.c |  3 +--
>
> This would be a patch where using scripts/git.orderfile to highlight the
> interface change first would make review a bit quicker :)
>
>> +++ 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.

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?

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

Thanks!