Re: [Qemu-devel] [PATCH v11 19/28] qapi: Change munging of CamelCase enum values

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> [hmm, wonder why scripts/get-maintainer.pl didn't loop in Gerd to the
> patch itself]
>
> On 11/11/2015 07:50 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> When munging enum values, the fact that we were passing the entire
>>> prefix + value through camel_to_upper() meant that enum values
>>> spelled with CamelCase could be turned into CAMEL_CASE.  However,
>>> this provides a potential collision (both OneTwo and One-Two would
>>> munge into ONE_TWO).  By changing the generation of enum constants
>>> to always be prefix + '_' + c_name(value).upper(), we can avoid
>>> any risk of collisions (if we can also ensure no case collisions,
>>> in the next patch) without having to think about what the
>>> heuristics in camel_to_upper() will actually do to the value.
>> 
>> This is the good part: the rules for clashes become much simpler.
>> 
>> Bonus: the implementation for detecting them will be simple, too.
>> 
>>> Thankfully, only two enums are affected: ErrorClass and InputButton.
>
> Visiting just InputButton in this email.
>
>> 
>> By convention (see CODING_STYLE), we use CamelCase for type names, and
>> nothing else.
>> 
>> Only enums violating this naming convention can be affected.  The bad
>> part: they exist.
>> 
>> InputButton has two camels: WheelUp and WheelDown.  The C enumeration
>> constants change from INPUT_BUTTON_WHEEL_UP/WHEEL_DOWN to
>> INPUT_BUTTON_WHEELUP/WHEELDOWN.  Not exactly an improvement, but one,
>> there are just 21 occurences in 11 files, and two, I think we can still
>> fix the enumeration to "lower case with dash", as it's only used by
>> x-input-send-event.
>
> The InputButton type has existed since 2.0; which is then part of the
> 'InputBtnEvent' struct, then the 'InputEvent' union, also since 2.0.  I
> can't easily tell if it was only used internally at that point, or if we
> exposed it through the command line (even if we didn't expose it through
> QMP);

As far as I can tell, we used it internally, and for tracing.

>       but I concur with your reading that in QMP it is only used via
> 'x-input-send-event' (since 2.2, but the x- prefix gives us freedom).
> [Oh, and I just spotted a typo; 'InputAxis' has a copy-and-paste doc
> typo calling it 'InputButton']
>
> If desired, I can prepare an alternate patch that adds the dash to the
> qapi enum definition, to see what we think.

If Gerd is fine with the rename, let's do it.

> But meanwhile, look at some of the lines in the patch:
>
>> diff --git a/ui/sdl.c b/ui/sdl.c
>> index 570cb99..2678611 100644
>> --- a/ui/sdl.c
>> +++ b/ui/sdl.c
>> @@ -469,8 +469,8 @@ static void sdl_send_mouse_event(int dx, int dy,
>> int x, int y, int state)
>>          [INPUT_BUTTON_LEFT] = SDL_BUTTON(SDL_BUTTON_LEFT),
>>          [INPUT_BUTTON_MIDDLE] = SDL_BUTTON(SDL_BUTTON_MIDDLE),
>>          [INPUT_BUTTON_RIGHT] = SDL_BUTTON(SDL_BUTTON_RIGHT),
>> -        [INPUT_BUTTON_WHEEL_UP] = SDL_BUTTON(SDL_BUTTON_WHEELUP),
>> -        [INPUT_BUTTON_WHEEL_DOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN),
>> +        [INPUT_BUTTON_WHEELUP] = SDL_BUTTON(SDL_BUTTON_WHEELUP),
>> +        [INPUT_BUTTON_WHEELDOWN] = SDL_BUTTON(SDL_BUTTON_WHEELDOWN),
>
> Since SDL already spells the names without space, it's not the end of
> the world if we do likewise.

Good point.

Even if we adopt SDL's spelling WHEELUP and WHEELDOWN, I'd still prefer
to downcase the QAPI names for consistency with the rest of QAPI.